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.

Sign up for GitHub

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

Jump to bottom

fix build with llvm16+ where --no-undefined-version is the default #1200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix build with llvm16+ where --no-undefined-version is the default #1200

wants to merge 1 commit into from

Conversation

svmhdvn
Copy link
Contributor

@svmhdvn svmhdvn commented Dec 15, 2023

LLVM 16+ enables the --no-undefined-version flag in LLD by default in commit llvm/llvm-project@241dbd3. This causes many build errors of the form: error: version script assignment of <VERSION> to symbol <SYMBOL> failed: symbol not defined in heimdal.

This patch enables preprocessing of the version-script.map files so that we can conditionally export symbols in version scripts based on #define values in config.h.

@svmhdvn svmhdvn force-pushed the fix-llvm-17 branch 3 times, most recently from a4afc1a to bca629e Compare December 15, 2023 20:55
@nicowilliams
Copy link
Contributor

Hey, this is very neat! I need to spend some time reviewing this, but yeah, the idea is sound, and we're almost certainly going to need to be doing this in the longer term as the linker-editors become more strict.

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Jan 13, 2024

Sounds good, I do have some bandwidth to push this PR to submission so let me know whenever you get the time.

nicowilliams
nicowilliams reviewed Jan 13, 2024
View reviewed changes
lib/roken/version-script.mapin
rk_freehostent;
rk_freeifaddrs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be `#ifdef``'ed somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at

heimdal/lib/roken/getifaddrs.c

Line 56 in cb9a130

#ifdef HAVE_IFADDRS_H
, is that supposed to be #ifdef or #ifndef? I think I can use that ifdef right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question. I'm not sure, but I think you're right. @jaltman may know.

nicowilliams
nicowilliams reviewed Jan 13, 2024
View reviewed changes
lib/roken/version-script.mapin
rk_localtime_r;
rk_memmem;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do use memmem()/rk_memmem().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted this because memmem is redirected to rk_smemmem. Take a look at

heimdal/lib/roken/roken.h.in

Line 505 in cb9a130

#ifndef HAVE_MEMMEM
.

Is this intentional? I don't find any other references to rk_memmem directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it gets renamed to rk_memmem(), so the three uses in kdc/connect.c are actually uses of rk_memmem() (but only when the OS/C library doesn't have a memmem()). That's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the annoyance and my possible misunderstanding of your comment, but could you point me to the definition of rk_memmem? Here's my understanding:

  • Renaming of memmmem to rk_smemmem happens here:

    heimdal/lib/roken/roken.h.in

    Lines 506 to 507 in cb9a130

    #define memmem rk_smemmem
    ROKEN_LIB_FUNCTION void * ROKEN_LIB_CALL memmem(const void *, size_t, const void *, size_t);

  • Definition of memmem (which gets renamed again to rk_smemmem by the previous macro) is here:

    heimdal/lib/roken/memmem.c

    Lines 38 to 41 in cb9a130

    #ifndef HAVE_MEMMEM
    #include "roken.h"
    ROKEN_LIB_FUNCTION void * ROKEN_LIB_CALL
    memmem(const void *haystack,

  • All calls of memmem happen in kdc/connect.c

Where does the renaming happen for memmem to rk_memmem? Note the difference of the letter s between rk_memmem and rk_smemmem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, where did that s come from? Well, that's clearly a typo. Now I'm curious how this works on Windows.

nicowilliams
nicowilliams reviewed Jan 13, 2024
View reviewed changes
lib/roken/version-script.mapin
@@ -219,8 +301,6 @@ HEIMDAL_ROKEN_2.0 {
rtbl_set_flags;
rtbl_set_prefix;
rtbl_set_separator;
signal;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need the signal() wrapper? And why isn't it called rk_signal()? @jaltman. Maybe we should just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep it removed for now then

@nicowilliams
Copy link
Contributor

I think this is pretty close. Thanks for doing this work!

@nicowilliams
Copy link
Contributor

image

image

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Jan 14, 2024

Can you give repro instructions for those errors? They look unrelated to my changes. Things are building successfully on my local system with LLVM 17.

Quick thing I noticed, I've accidentally added libedit's chartype.h local changes to this PR, deleting this now and repushing.

@nicowilliams
Copy link
Contributor

@svmhdvn you can see those in the Windows build errors in the checks that are being run on this PR. Just click on Details for each failed build, click on the failed action, click the search field, type error, then search backwards through the logs. There's also a log download option. Note that the full logs are not loaded into the page, so you can't reliably use the browser's search feature.

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Jan 14, 2024

How can I trigger the Windows build again? My PR doesn't seem to trigger it anymore.

@nicowilliams
Copy link
Contributor

How can I trigger the Windows build again? My PR doesn't seem to trigger it anymore.

See .github/workflows/windows.yml:

    pull_request:
      paths:
         - '!docs/**'
         - '!**.md'
         - '!**.[1-9]'
         - '**.[chly]'
         - '**.hin'
         - '**.in'
         - '**.pl'
         - '**.py'
         - '**.asn1'
         - '**.opt'
         - '**.w32'
         - '**/NTMakefile'
         - '**/COPYING'
         - '**/INSTALL'
         - '**/README*'
         - '.github/workflows/windows.yml'
         - '!appveyor.yml'
         - '!.travis.yml'

@svmhdvn
Copy link
Contributor Author

svmhdvn commented Jan 19, 2024

If the latest version of my PR doesn't trigger any windows builds due to the windows filter, then it shouldn't affect the windows build in any way right? Is this review otherwise complete?

@nicowilliams
Copy link
Contributor

I'll do a final review and merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicowilliams nicowilliams nicowilliams left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@svmhdvn @nicowilliams