This is an archive of the discontinued LLVM Phabricator instance.

libcxx: Don't apply ABI tags to extern "C" fns
ClosedPublic

Authored by ArsenArsen on Jan 23 2023, 4:05 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG21d9282ae2b6: libcxx: Don't apply ABI tags to extern "C" fns
Summary

GCC rejects ABI tags on non mangled functions, as they would otherwise
be a no-op. This commit replaces such instances with equivalent
_LIBCPP_HIDE_FROM_ABI constants but without ABI tags attached.

.../include/c++/v1/__support/musl/xlocale.h:28:68: error: 'abi_tag'
attribute applied to extern "C" declaration 'long long int
strtoll_l(const char*, char**, int, locale_t)'
   28 | strtoll_l(const char *__nptr, char **__endptr, int __base, locale_t) {
      |                                                                    ^

Bug: https://bugs.gentoo.org/869038

Diff Detail

Event Timeline

ArsenArsen created this revision.Jan 23 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 4:05 PM
ArsenArsen requested review of this revision.Jan 23 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 4:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Elaborated the commit message a bit. Didn't mean to send out the shortened
one. Sorry about that - still getting adjusted to Arcanist.

ArsenArsen edited the summary of this revision. (Show Details)Jan 23 2023, 4:16 PM

Rebase patch.

Apply formatting patch from CI.

ldionne accepted this revision.Jan 23 2023, 5:27 PM
ldionne added a subscriber: ldionne.

LGTM w/ green CI and suggestions applied.

libcxx/include/__config
626–628

Can you define this below with _LIBCPP_HIDE_FROM_ABI_VIRTUAL?

628

Please don't modify this macro -- I know it's possible to "reuse" _LIBCPP_HIDE_FROM_ABI_C to define _LIBCPP_HIDE_FROM_ABI, but IMO adding a layer of indirection there makes things harder to understand. And since that is already extremely tricky, I don't think we should :-).

This revision is now accepted and ready to land.Jan 23 2023, 5:27 PM
ArsenArsen added inline comments.Jan 23 2023, 5:46 PM
libcxx/include/__config
628

My reasoning for this was that the _C macro should reflect exactly the usual macro, save for the ABI tag, and so, reflecting that in the code seemed to make sense.

To save bouncing revisions around, I'll update with this changed, and we can just pick one of the revisions later.

This is also why this macro was awkwardly fit between this comment and conditional block, btw ;D

Apply suggestions.

This revision was landed with ongoing or failed builds.Jan 24 2023, 11:35 PM
This revision was automatically updated to reflect the committed changes.