-
Notifications
You must be signed in to change notification settings - Fork 577
openbsd: ensure we link to the built libperl.a, not the system libperl.a #23265
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
base: blead
Are you sure you want to change the base?
Conversation
Issue Perl#22125 detected that we weren't linking the correct library with the embedded test with gcc on OpenBSD, so add an API to perform a sanity check by comparing the size of the perl interpreter structure (or its size if it was a structure) and expected perl API version between those seen in the binary and those compiled into libperl.
When building with gcc, lib/ExtUtils/t/Embed.t would link against the system libperl.a rather than the newly built libperl.a. Due to the limited API used by the sample code this typically didn't crash, but some configuration changes could result in a crash or a link error. For OpenBSD, change the link options to more closely match those used when building the perl executable, which results in linking against the correct library. Fixes Perl#22125
@tonycoz, thanks for continuing to investigate this. Could you rebase your smoke-me branch and push this new code to that branch? That way, @cjg-cguevara's smoker will pick up the branch. @afresh1, can you take a look at this p.r.? Thanks. |
@tonycoz: I fetched your p.r. and built a
This configuration was the first one I discussed in #22125. All tests PASSed. So that's a very good sign. I will want to re-test this with |
I pushed it to the smoke-me, https://perl.develop-help.com/?b=smoke-me%2Ftonyc%2F22125-openbsd-embed we're just waiting for results now. |
I've also gotten a PASS with the following configuration:
Am beginning a smoke-test run with my customary configurations. That will take several hours. Note that I haven't looked at the code changes at all -- only the test results. |
I should be able to run tests with this. Will have to remember which architectures I have that still use gcc :-) |
object was built with and the perl that C<libperl> was built with. | ||
|
||
This can be used to ensure that these match and produces a more | ||
diagnosable than random crashes and mis-behaviour. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a word or two missing after diagnosable
. diagnosable behavior
, perhaps?
Intuitively, it seems wrong to me that this is OpenBSD specific code. Are we sure we're fixing this in the right place? |
Smoke testing didn't find anything else tripping the added version check. There may be other systems that trip this, but I'd rather not risk breaking this test for other systems by making the change for other systems too without evidence that it's needed. |
Both points seem reasonable. We (PSC) are thinking we should ship this patch as is for now, but we also want to pick this up early next cycle and investigate whether we can make this change across the board without limiting it to OpenBSD. |
Yeah that sounds reasonable to me. |
Time got away from me, it won't be this week. |
When building with gcc, lib/ExtUtils/t/Embed.t would link against the system libperl.a rather than the newly built libperl.a.
Due to the limited API used by the sample code this typically didn't crash, but some configuration changes could result in a crash or a link error.
For OpenBSD, change the link options to more closely match those used when building the perl executable, which results in linking against the correct library.
Fixes #22125
Also adds a test to ensure we are linking against the correct libperl.
Smoke reports from before a trivial rebase at https://perl.develop-help.com/?b=smoke-me%2Ftonyc%2F22125-openbsd-embed