Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: use PIE objects for non-PIC static libraries if b_pie=true #7760

Merged
merged 1 commit into from
Oct 18, 2020

Conversation

bonzini
Copy link
Contributor

@bonzini bonzini commented Sep 18, 2020

If static_library is used for a convenience library (e.g. for link_whole), the library should in principle not need position independent code. However, if the executables that the libraries is linked to are PIE, the non-PIC objects in the static library will cause linker errors. To avoid this, obey b_pie for static libraries if either b_staticpic=false or they use pic: false.

Without this patch, a project that only uses static libraries as convenience libraries cannot use b_staticpic=false, because the correct setting would be, so to speak, b_staticpic=$b_pie. This causes a massive slowdown on some QEMU benchmarks (up to 20%). We can work it around for now while we still have wrappers upon wrappers around Meson, but in the long run this should be fixed.

But actually, this patch also provides a performance improvement if b_pie=true, because b_staticpic=true b_pie=true is also slower than the b_staticpic=false b_pie=true combo that is now fixed. This is because -fPIC produces worse code than -fPIE. On the aforementioned benchmark:

b_staticpic=false b_pie=false               0.5s
b_staticpic=true b_pie=true                 0.6s
---
b_staticpic=false b_pie=true unpatched      linking fails
b_staticpic=false b_pie=true patched        0.53s

@bonzini bonzini changed the title build: use PIE objects for static libraries if pic: false build: use PIE objects for static libraries if pic: false but b_pie=true Sep 18, 2020
@bonzini bonzini changed the title build: use PIE objects for static libraries if pic: false but b_pie=true build: use PIE objects for static libraries if b_staticpic=false but b_pie=true Sep 18, 2020
@bonzini bonzini force-pushed the static-library-pie branch 3 times, most recently from f35f5d2 to 6bdec78 Compare September 19, 2020 10:33
@jpakkane
Copy link
Member

Just a note that traditionally these sort of behaviour changes have been a bit dangerous. Usually someone's project breaks. Are there any known cases where this might happen?

@bonzini
Copy link
Contributor Author

bonzini commented Sep 24, 2020

I don't know any, in fact I'm trying to make Meson more conservative: currently it's assuming that b_staticpic=false static libraries are only ever used in position-dependent code, while my patch makes the libraries acceptable for PIEs as well.

bonzini added a commit to bonzini/qemu that referenced this pull request Sep 30, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200919155639.1045857-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
@bonzini
Copy link
Contributor Author

bonzini commented Oct 1, 2020

Ping?

@jpakkane
Copy link
Member

jpakkane commented Oct 1, 2020

This requires thinking about the implications fairly thoroughly. I'll try to get that done this weekend.

@jpakkane
Copy link
Member

jpakkane commented Oct 4, 2020

One thought that came up was that maybe b_staticpic is the wrong thing to do. What if we could change it to, say, b_statictype that could have values none, pic or pie? It would seem to express the intent better. But would it help in solving the issue?

@bonzini
Copy link
Contributor Author

bonzini commented Oct 4, 2020

It would not help because there is no way to express that you want b_statictype equal to none if b_pie=false, and equal to pieifb_pie=yes`.

I think you are seeing this as an optimization that allows one to use b_staticpic=false; instead I see it as a bug fix, because b_staticpic=false or pic: false are the right choice more often than not, but they're currently broken if b_pie=true. Imagine if the inverse of b_staticpic were named b_static_libraries_linked_to_executables_only: that makes it more obvious if a project wants it true or false---but then, we have the bug that b_staticpic=false it does not work if b_pie=true.

(Related to this would it make sense to deprecate installing static_libraries if b_staticpic is false?).

@jpakkane
Copy link
Member

jpakkane commented Oct 5, 2020

I guess the problem here is that the original design for b_staticpic was too limited. Instead of a boolean it should have had the values alwayspic, never, alwayspie and pieifb_pieistrue (it should also have a better name in that case, but I digress). The question is whether we could achieve that without breaking backwards compatibility (too much).

@bonzini
Copy link
Contributor Author

bonzini commented Oct 5, 2020

Yeah that would be more precise, but then you'd also want a pie keyword argument to static_library and I think that would be overkill.

In my opinion even the pie keyword argument for executables is overkill; b_pie should be enough. It was probably done for consistency with pic in static libraries but the two are different. Having pic for static libraries makes sense because convenience libraries for executables are a common usage of static_library, but there's no equivalent use of pie`.

This is also why I am okay with a boolean b_staticpic: if you remove pie from executable, then the never and alwayspie cases aren't needed and the remaining two might as well be true and false.

bonzini added a commit to bonzini/qemu that referenced this pull request Oct 6, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200919155639.1045857-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to bonzini/qemu that referenced this pull request Oct 7, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200919155639.1045857-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
stsquad pushed a commit to stsquad/qemu that referenced this pull request Oct 8, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200919155639.1045857-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200924092314.1722645-57-pbonzini@redhat.com>
…b_pie=true

If static_library is used as a convenience library (e.g. for link_whole)
it should in principle not need position independent code.
However, if the executables that the libraries is linked to are PIE,
the non-PIC objects in the static library will cause linker errors.
To avoid this, obey b_pie for static libraries if either b_staticpic=false
or they use "pic: false".

Without this patch, QEMU cannot use b_staticpic, which causes a slowdown
on some QEMU benchmarks up to 20%.
stsquad pushed a commit to stsquad/qemu that referenced this pull request Oct 9, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200924092314.1722645-57-pbonzini@redhat.com>
Message-Id: <20201007160038.26953-2-alex.bennee@linaro.org>
bonzini added a commit to bonzini/qemu that referenced this pull request Oct 9, 2020
Because most files in QEMU are grouped into static libraries, Meson conservatively
compiles them with -fPIC.  This is overkill and produces slowdowns up to 20% on
some TCG tests.

As a stopgap measure, use the b_staticpic option to limit the slowdown to
--enable-pie.  mesonbuild/meson#7760 will allow
us to use b_staticpic=false and let Meson do the right thing.

Reported-by: Ahmed Karaman <ahmedkrmn@outlook.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200919155639.1045857-1-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
@jpakkane
Copy link
Member

Thinking about this more, pie is actually unnecessary because you can do override_options: b_pie=false`. It's not as convenient, though.

There are really three cases that I can see.

  • PIC, if the library ends up in a shared library
  • PIE, if the library ends up in an executable that needs to be PIE
  • nothing, for absolute performance (according to the Internet, PIE has a ~10% performance penalty)

On some platforms (mac?) the last two are the same, all code is at least PIE.

You can't know where a library will be used when it is declared so barring advances in clairvoyance we can't autodetect that. So let's speculate some instead. If we had b_pie as currently and a kwarg pi_type for static libraries that takes values none, pic or pie, would that be sufficient for all cases? From what I can see:

  • if you know a static lib eventually goes to a shared lib, you can do `pi_type: 'pic'
  • Toggling b_pic would automatically make static libraries switch to the desired picness
  • for individual executables you could override b_pic as necessary

The downside is that to match the current behaviour, the default value should be pic.

@bonzini
Copy link
Contributor Author

bonzini commented Oct 12, 2020

You can't know where a library will be used when it is declared so barring advances in clairvoyance we can't autodetect that

Right, this is why I would like to warn on installing pic: false libraries. Those are meant to be linked with executables (in the current project) because once the static library it's installed it could end up linked with a shared library. And in order to link with executables, non-PIC libraries must obey b_pie.

(More precisely therefore this PR should be "use PIE objects for non-PIC static libraries if b_pie=true").

If we want to have PIE libraries in non-PIE projects or vice versa, adding override_options to build targets other than executable is probably the way to go.

The downside is that to match the current behaviour, the default value should be pic.

Yeah, the problem with pi_type is that it preserves the "flat" ternary choice while it's actually a decision tree:

  • do you every want to link the static library to a shared library? (pic, b_staticpic)
  • if not, are your executables of interest PIE? (b_pie with override_options and/or pie—none of the two exists in static_library but it can be added later)

I have no problem with the default for b_staticpic being the safe value, i.e. true. On the other hand the "I want absolute performance for a binary or two but otherwise b_pie=true" case seems pretty contrived.

@bonzini bonzini changed the title build: use PIE objects for static libraries if b_staticpic=false but b_pie=true build: use PIE objects for non-PIC static libraries if b_pie=true Oct 12, 2020
@jpakkane
Copy link
Member

Just in case: this MR gets an exception to get merged in after RC1 (if necessary).

@jpakkane
Copy link
Member

I went through this again and it clearly improves the situation for an important use case. I could not come up with a case where it would break things. If they exist, hopefully they crop up during the RC (though the pessimist in me expects that to only happen after the actual release :( ). f that happens we may need to revert this, but that's a different problem then.

In case problems occur, we should consider overhauling the pic/pie options under a single toggle. The current split is confusing.

@jpakkane jpakkane merged commit 021d242 into mesonbuild:master Oct 18, 2020
@bonzini bonzini deleted the static-library-pie branch October 19, 2020 08:07
yangzhon pushed a commit to intel/qemu-sgx that referenced this pull request Sep 8, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request Sep 20, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
bonzini added a commit to qemu/qemu that referenced this pull request Oct 5, 2021
The update to 0.57 has been delayed due to it causing warnings for
some actual issues, but it brings in important bugfixes and new
features.  0.58 also brings in a bugfix that is useful for modinfo.

Important bugfixes:

- 0.57: mesonbuild/meson#7760, build: use PIE
objects for non-PIC static libraries if b_pie=true

- 0.57: mesonbuild/meson#7900, thus avoiding
unnecessary rebuilds after running meson.

- 0.58.2: mesonbuild/meson#8900, fixes for
passing extract_objects() to custom_target (useful for modinfo)

Features:

- 0.57: the keyval module has now been stabilized

- 0.57: env argument to custom_target (useful for hexagon)

- 0.57: Feature parity between "meson test" and QEMU's TAP driver

- 0.57: mesonbuild/meson#8231, allows bringing
back version numbers in the configuration summary

- 0.59: Utility methods for feature objects

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants