Opened 2 years ago

Closed 15 months ago

Last modified 11 months ago

#23846 closed enhancement (implemented)

Option to build Tor with -fPIC (Use libtool for building shared library?)

Reported by: hellais Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-mobile, s8-api, 034-triage-20180328, 034-included-20180402, 034-roadmap-subtask, 035-roadmap-subtask, 035-triaged-in-20180711
Cc: darkk, brade, mcs, ahf, nickm, sbs, chelseakomlo Actual Points: 2
Parent ID: #25510 Points:
Reviewer: ahf Sponsor: Sponsor8

Description

It seems like it's ideal to use libtool for generating a shared library. Simone made some progress on getting this working here: https://github.com/bassosimone/mkok-onion-event/pull/2/files.

Child Tickets

Change History (49)

comment:1 Changed 2 years ago by hellais

Cc: sbs added

comment:2 Changed 2 years ago by nickm

Sponsor: Sponsor8

comment:3 Changed 2 years ago by ahf

Keywords: s8-api added

comment:4 Changed 23 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Deferring various "new"-status enhancement tickets to 0.3.4

comment:5 Changed 21 months ago by sbs

I have [this patch set](https://gist.github.com/bassosimone/72f95974881812695b55f9d886c1ba2d) where I basically:

  • modify the build system of tor to make convenience libtool libraries as opposed to convenience .a archvies
  • add an extra rule to create from such convenience libraries a single .la / .a archive

This simplifies integrating tor because, after this change, there is a single library to embed rather than many. Also such setup seems more robust because the composition of many libraries into a single one is done by Tor build system rather than by integrators.

comment:6 Changed 21 months ago by sbs

Owner: set to sbs
Status: newassigned

comment:7 Changed 21 months ago by sbs

Status: assignedneeds_review

comment:8 Changed 21 months ago by sbs

@nickm: I previously mentioned to you that tests were not passing after this changes, but it turns out this was transient (i.e., I have just built everything again and run tests and it is now okay). Since Wi-Fi was very bad in my hotel room, I am going to assume that that was the cause.

comment:9 Changed 21 months ago by asn

Reviewer: ahf

comment:10 Changed 21 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 21 months ago by ahf

Status: needs_reviewneeds_revision

I think overall this looks like it's fine, but I would like you to do the following:

  1. Fork the https://github.com/torproject/tor.git repository to your own account.
  2. Push your patches to a branch in your fork.

That should allow us to see if everything is going well with our Travis CI. Around build-system changes it is probably a very good idea to be sure we don't break anything here.

comment:12 Changed 21 months ago by sbs

Status: needs_revisionneeds_review

comment:13 Changed 21 months ago by nickm

Keywords: 034-included-20180402 added

comment:14 Changed 21 months ago by catalyst

Thanks for the pull request! It's good to see Travis passing.

I'm not very familiar with libtool, so sorry if I miss anything obvious. This seems to switch from building static libraries to building shared libraries, if I'm reading the rust warning correctly. Is this something we want? It could make the build more fragile in some ways.

Do we know how this will affect Windows builds? The .$(OBJEXT) vs .lo change makes me nervous.

I also made a minor comment on the pull request about verbosity.

comment:15 Changed 21 months ago by ahf

Reviewer: ahfnickm

Changing reviewer to Nick as per discussion on #tor-dev

comment:16 Changed 21 months ago by sbs

catalyst: thanks for the comments. I have starting replying to the easiest ones directly in the PR at github (https://github.com/torproject/tor/pull/35#discussion_r178749931) and will address the other comments later today. I think I have a sense of how to reply to all comments but I'd rather read the manuals and double check that I understand correctly.

comment:17 Changed 21 months ago by sbs

Addressing more comments from catalyst:

Do we know how this will affect Windows builds? The .$(OBJEXT) vs .lo change makes me nervous.

What build system is used to build Tor on Windows? I see that there is a Makefile.nmake and a Makefile.am and the documentation mentions mingw (but mainly below contrib).

Probably both Makefile.am and Makefile.nmake are used, then:

If tor is built using MS tooling and Makefile.nmake, then this change has no impact.

Otherwise, to better understand, I tried to build libevent (which uses libtool) on Windows using MSYS2 and mingw-w64 and found that it produced .lo files. I also searched online and it seems the documentation of automake, when specifically talking about files produced with and without libtool, does not mention any special variable for the extension of libtool-produced files; rather it uses file.lo, while it specifically mentions file.$(OBJEXT) [1] [2].

[1] https://www.gnu.org/software/automake/manual/html_node/Objects-created-both-with-libtool-and-without.html

[2] https://www.gnu.org/software/automake/manual/html_node/Program-and-Library-Variables.html

comment:18 Changed 20 months ago by nickm

Keywords: 034-roadmap-subtask added
Parent ID: #23684#25510

comment:19 Changed 20 months ago by catalyst

sbs: Thanks for the research and information! This looks like I'll need to do more background reading to properly evaluate it. The linking a shared library into a static Rust library still sounds concerning to me, so if anyone else could learn more about it and report back, that would help a lot.

comment:20 Changed 20 months ago by isis

It's possible to have rust build shared object libraries instead of static, see the "crate_type" argument in our Cargo.tomls and also this reference page (we'd just have to add "dylib" to the lists, I think, or "cdylib" if it's being linked to C/Java/Swift/etc). We can even tell it to build both, so desktop systems can continue to use static linking and I guess mobile can do dynamic?

Last edited 20 months ago by isis (previous) (diff)

comment:21 in reply to:  17 ; Changed 20 months ago by nickm

So, I've looked over the codebase and made a couple of changes in a branch called "libtool_hack", including a workaround for the Rust warning issue. (It's only a plausible legitimate workaround if Rust builds with -fPIC though -- does it?)

Here are the open issues I want to check, if we can:

  • Does this work on windows? (Only the Makefile.am build is supported)
  • Does "make distcheck" still work? (It does without Rust. But with Rust, "make distcheck" appears to be broken in master already without this patch. Fixing.)
  • How horrible is the above Rust hack?

One other issue here is that since none of the library are installed, no real shared libraries are actually built. (You can verify this yourself by noticing that no .so files are created anywhere.) I'm _thinking_ that this is intentional, but I'm

comment:22 in reply to:  21 Changed 20 months ago by nickm

Replying to nickm:

  • Does "make distcheck" still work? (It does without Rust. But with Rust, "make distcheck" appears to be broken in master already without this patch. Fixing.)

Did that part in #25732

comment:23 Changed 20 months ago by ahf

Reviewer: nickmahf

comment:24 in reply to:  21 Changed 20 months ago by Hello71

Replying to nickm:

So, I've looked over the codebase and made a couple of changes in a branch called "libtool_hack", including a workaround for the Rust warning issue. (It's only a plausible legitimate workaround if Rust builds with -fPIC though -- does it?)

yes. it is highly unlikely that they will stop, due to the numerous benefits of PIC and negligible costs, but if so, there will be loud build time errors and we can figure it out then. we can safely assume that there will always be a way to turn PIC on, since otherwise it would be impossible to make even pure-Rust shared libraries.

  • How horrible is the above Rust hack?

personally I think we should just ignore it. this hack will probably cause more problems than it solves. I believe the warning only applies to obsolete Unix systems that Rust does not and will never support anyways. as I said above, if there is any breakage it will be loud and at build time anyways.

comment:25 Changed 20 months ago by Hello71

or we could print a "ignore the following non-portable message, this is portable everywhere Rust runs". or we could move to a non-autotools system.

comment:26 Changed 20 months ago by sbs

Regarding linking Rust generated code and -fPIC code, there is this additional data point [edit: in addition to what Hello71 already mentioned]. The rust developer themselves acknowledge that they use -fPIC for static libraries on github, explain why that is the default, and how to disable -fPIC [1].

(I originally wrote a much longer comment but I would like to do more research before writing libtool-related statements of which I am not 100% sure yet.)

I would perhaps add that, after having worked a little more on this issue, I have a sense that what we actually need for mobile is mainly a _single_ .a archive compiled with -fPIC code and a header. At least, this is exactly what we need on Android (for iOS I need to double check whether we can get away without -fPIC, but since Android needs -fPIC, I'd say we need -fPIC). [edit: this paragraph should explain why the diff doesn't make a shared library]

In this regard, the main reason to use libtool is that it seems (based on my research, can provide more pointers if needed) the most portable, less hackish way to assemble several .a files (through .la or, with the aforementioned warning, using .a generated by Rust).

I'm hoping to dedicate more on this ticket this week and would like to do the following:

  1. make a simple iOS and Android apps using the .a library and confirm it works
  1. make sure it also works on Windows 10 (which is what I have :-)

Regarding the latter point, what is the reference build system? In addition to MSVC, I also use MSYS2 with mingw-w64 on Windows. Would MSYS2 be okay as a build system to test?

Also, is there interest in adding CI using AppVeyor? If so, I can probably try to add an appveyor.yml to my fork on github and try building with MSYS2. Then, if it's okay, also that patch can be upstreamed. Would that be of interest?

Final question: integration would be further simplified if there was an autoconf/automake rule to install the static library and the header. Would a third diff adding that be accepted? (We have scripts that compile and install all dependencies in a specific location and not having rules to install means writing bash to copy the library and headers -- not a big deal but less smooth.)

[1] https://github.com/rust-lang/rust/issues/27142

Last edited 20 months ago by sbs (previous) (diff)

comment:27 Changed 20 months ago by nickm

To try to answer your questions: we use msys+mingw for our windows builds; we don't support or recommend MSVC. MSYS2 should be fine.

I'd love appveyor support, but there's already a ticket for that, with some work in progress: see #25549. They aren't done yet; maybe they could use some help.

I'd be fine having an automake rule to install the static library and header, but it would need to be off-by-default, and enabled with --enable-library-install or something.

comment:28 Changed 20 months ago by ahf

I think it would be interesting to hear from sbs if the current code works for what you need to use it for?

Is there a reason in our .gitignore file that we explicitly list the *.lib and *.la files and don't glob them like we do with *.o and *.lo?

I think overall the patches in Nick's libtool_hack branch looks good. The above questions are the only ones I have so far.

comment:29 Changed 20 months ago by ahf

Status: needs_reviewneeds_information

comment:30 Changed 20 months ago by nickm

I think the reason for ignoring .lib and .la files rather than using a glob is simply that we were not using a glob for .a files.

comment:31 Changed 19 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

comment:32 Changed 19 months ago by chelseakomlo

Cc: chelseakomlo added

comment:33 Changed 18 months ago by nickm

Keywords: 035-roadmap-subtask added

comment:34 Changed 17 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:35 Changed 17 months ago by hellais

I started using the branch of nickm called libtool_hack to build a dylib on macOS.

In order to get it to actually build a .dylib on macOS I had to apply this patch: https://github.com/hellais/tor/commit/163d22294df6674411c3bfe6c46868df995a6bb2.

I think it would be very useful to have the work in this branch done and updated to the latest master so that we can better test tor_api.h (it's easier to write tests using ffi in python that by writing c code).

Based on this branch I wrote a simple failing test that makes tor crash when the tor_run_main() function is called twice. See: https://gist.github.com/hellais/b56043d57eb5be885958e80b3665bfe2

I am not sure if that is due to not all the fixes being part of that branch. In the meantime I have started to forward port the changes in this branch to the latest master, but there are quite a few conflicts that I am not entirely sure what is the best approach to follow to resolve them.

comment:36 Changed 17 months ago by hellais

For the purpose of testing I didn't end up using this branch as it's quite tricky to have it work with the latest master.

It would be great if these patches could be forward ported to the latest master so we can easily build a shared library with libtool.

comment:37 Changed 17 months ago by sbs

Hello! My apologies for a late reply and having neglected this ticket for quite some time. I was rightfully prodded by hellais to explain what we exactly need and I understand that any way in which this is simpler is easier for you guys to maintain. Long story short, what we need is the following: a single static library compiled with -fPIC and a header. (I do understand that having a shared library is probably a nice value proposition for people that perhaps wants to link to Tor, but I can also feel that it would complicate the build, so, since we do not need it, I would not advocate for it.) Thanks! :-)

comment:38 Changed 17 months ago by sbs

Addition: as far as I can tell [1], there is no portable, non hackish way to make a single static library out of many static libraries. I remember discussing this with ahf and concluding that probably using libtool was a good solution. It is a also a bit of a bummer that what we need libtool for is to assemble the static library from the many other libraries produced by the build system. I think I initially considered making another diff that modified the build system to make a single static library, but then I started wondering about rust integration and then suddenly a libool rule for assembling a single static library from rust static libraries and the many tor folders was not that bad. Still I am unhappy of libtool because it makes the build slower (even with --disable-shared on by default IIRC it was a bit slower).

[1] https://stackoverflow.com/a/23621751

Last edited 17 months ago by sbs (previous) (diff)

comment:39 Changed 17 months ago by nickm

Owner: changed from sbs to nickm
Status: needs_informationaccepted

comment:40 in reply to:  37 Changed 17 months ago by nickm

Replying to sbs:

Hello! My apologies for a late reply and having neglected this ticket for quite some time. I was rightfully prodded by hellais to explain what we exactly need and I understand that any way in which this is simpler is easier for you guys to maintain. Long story short, what we need is the following: a single static library compiled with -fPIC and a header. (I do understand that having a shared library is probably a nice value proposition for people that perhaps wants to link to Tor, but I can also feel that it would complicate the build, so, since we do not need it, I would not advocate for it.) Thanks! :-)

This is much easier, and we can probably get it done in 0.3.5.

I think we can get away with not using libtool here, since although the ar tricks described on that link are not universal, the easiest ones ought to be something we can hack together.

comment:41 Changed 17 months ago by nickm

One question: Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?

Right now, you can get a complete list of all the libraries you need to link for tor, in the correct order, by running "make show-libs". It's what we use in a few other places for external things like the clusterfuzz fuzzer that need to link "everything in tor".

That way instead of saying "tor/src/or/libtor_complete.a" in your linking command, you'd say "(cd tor && make show-libs)".

I suggest this instead of a single library because there are issues with Windows here, because Cargo builds its libraries as .lib rather than .a.

comment:42 Changed 17 months ago by sbs

Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?

Yes, that would be acceptable!

[...] all the libraries you need to link for tor, in the correct order,

This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.

Is is correct to say that the reason why different .a libraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?

[...] because Cargo builds its libraries as .lib rather than .a.

Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.

Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?

Thank you!

comment:43 in reply to:  42 Changed 17 months ago by Hello71

Replying to sbs:

Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?

Yes, that would be acceptable!

[...] all the libraries you need to link for tor, in the correct order,

This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.

Is is correct to say that the reason why different .a libraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?

As I understand it, it's just an easy way to implement "module-like" functionality in autotools, since variable order is a problem (autotools doesn't support anything like the CMake "object library" function).

[...] because Cargo builds its libraries as .lib rather than .a.

Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.

Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?

hey, I'm finally qualified to answer a question! Tor still uses mostly mingw to compile on Windows. MSVC is supported-ish but AFAIK no core Tor devs use it and Tor Browser still uses mingw. to use Rust on Windows, you must currently select to download the MSVC or the mingw version (which I assume are incompatible). Tor currently uses whatever you have installed, which now that I'm thinking about it, someone (hopefully not me) should write a configure test to ensure is linkable with the selected C toolchain, but it doesn't do that now.

comment:44 in reply to:  42 Changed 17 months ago by nickm

Summary: Use libtool for building shared libraryOption to build Tor with -fPIC (Use libtool for building shared library?)

Replying to sbs:

Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?

Yes, that would be acceptable!

[...] all the libraries you need to link for tor, in the correct order,

This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.

Is is correct to say that the reason why different .a libraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?

It's some of what you said here (different compiler flags); somewhat an attempt at enforcing modularity; and somewhat because some of our tools and testing programs only need smaller portions of the Tor codebase.

[...] because Cargo builds its libraries as .lib rather than .a.

Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.

Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?

I believe that mingw can link against .lib libraries; I don't know whether this is a full-compability thing, or whether the compatibility is partial.

Per https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SupportedPlatforms , msvc is still officially "Unsupported": We'll take clean patches to make it work, but we don't test with it or try hard to keep it working ourselves.

comment:45 Changed 16 months ago by sbs

Thanks nickm and Hello71 for your replies :-). Given that nickm has indicated an alternative approach that is good enough for our goals, I believe this can probably be closed. We will be using make show-libs.

comment:46 Changed 15 months ago by nickm

Status: acceptedneeds_review

Minimal version for the -fPIC issue is ticket23846; PR at https://github.com/torproject/tor/pull/321

comment:47 Changed 15 months ago by dgoulet

Status: needs_reviewneeds_information

Quick question on the PR. Else lgtm!

comment:48 Changed 15 months ago by nickm

Resolution: implemented
Status: needs_informationclosed

Okay -- I've added comments to explain my thinking there, and merged to master. Thanks!

comment:49 Changed 11 months ago by nickm

Actual Points: 2
Note: See TracTickets for help on using tickets.