This is an archive of the discontinued LLVM Phabricator instance.

AST: Move __va_list tag back to std conditionally on AArch64.
ClosedPublic

Authored by pcc on Jan 6 2022, 3:37 PM.

Details

Summary

In post-commit feedback on D104830 Jessica Clarke pointed out that
unconditionally adding va_list to the std namespace caused namespace
debug info to be emitted in C, which is not only inappropriate but
turned out to confuse the dtrace tool. Therefore, move
va_list back
to std only in C++ so that the correct debug info is generated. We
also considered moving __va_list to the top level unconditionally
but this would contradict the specification and be visible to AST
matchers and such, so make it conditional on the language mode.

To avoid breaking name mangling for __va_list, teach the Itanium
name mangler to always mangle it as if it were in the std namespace
when targeting ARM architectures. This logic is not needed for the
Microsoft name mangler because Microsoft platforms define va_list as
a typedef of char *.

Depends on D116773

Diff Detail

Event Timeline

pcc created this revision.Jan 6 2022, 3:37 PM
pcc requested review of this revision.Jan 6 2022, 3:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 3:37 PM
jrtc27 added a comment.EditedJan 7 2022, 5:39 AM

The fact that va_list is in the std namespace does leak out into __builtin_dump_struct, possibly the odd other place, and of course to external AST consumers.

I think it'd make sense to keep ASTContext as putting it in the std namespace for C++ (like it does for Arm, and used to for AArch64) but also have ItaniumMangle override it to the std namespace so that non-C++ gets the right mangling? Otherwise the AST and __builtin_dump_struct won't match the Arm spec.

pcc added a comment.Jan 7 2022, 10:29 AM

I'm not aware of any of those places causing an actual problem though? The AST isn't a stable interface, and builtin_dump_struct is for debugging purposes only. I would expect debug info consumers to be able to handle va_list in the global namespace as this is the status quo for C.

So I'm somewhat inclined to do the simple thing here first, and then look at making things more conditional if a problem comes up.

The fact that va_list is in the std namespace does leak out into __builtin_dump_struct, possibly the odd other place, and of course to external AST consumers.

It can also leak out in funny places like AST dumping (to text, but more interestingly, to JSON). But the one place that may be observable and really matters (that I can think of) are AST matchers that get used by clang-tidy.

I think it'd make sense to keep ASTContext as putting it in the std namespace for C++ (like it does for Arm, and used to for AArch64) but also have ItaniumMangle override it to the std namespace so that non-C++ gets the right mangling? Otherwise the AST and __builtin_dump_struct won't match the Arm spec.

I think this could make some sense. We typically try to have the AST reflect the reality of the program, and from that perspective, I think I would expect there to be a std namespace component for this in the AST if the spec calls for one even when obtaining the type through stdarg.h instead of cstdarg.

jrtc27 added a comment.Feb 9 2022, 5:50 AM

Ping? I'd really like to get this fixed in 14.

pcc updated this revision to Diff 407227.Feb 9 2022, 11:36 AM
pcc retitled this revision from AST: Move __va_list tag to the top level on ARM architectures. to AST: Move __va_list tag back to std conditionally on AArch64..
pcc edited the summary of this revision. (Show Details)

Make it conditional

jrtc27 added inline comments.Feb 17 2022, 6:45 AM
clang/lib/AST/ItaniumMangle.cpp
641

Could use isAArch64 (which then also picks up aarch64_32)

642

Could use isARM. Does this also need to care about isThumb, or has that been normalised to Arm with a T32 subarch?

pcc updated this revision to Diff 409716.Feb 17 2022, 10:39 AM

Use isARM() etc

pcc marked 2 inline comments as done.Feb 17 2022, 10:43 AM
jrtc27 accepted this revision.Feb 17 2022, 10:50 AM

Thanks, looks good to me

This revision is now accepted and ready to land.Feb 17 2022, 10:50 AM

Only a small nit from me.

clang/lib/AST/ASTContext.cpp
8555–8556
jrtc27 added inline comments.Feb 17 2022, 11:18 AM
clang/lib/AST/ASTContext.cpp
8555–8556

Hm, the advantage of leaving it as it is is then it completely reverts D104830 (ignoring the added CFI test) rather than being written in a slightly different but equivalent way. Don't really care either way myself though, both have their merits.

aaron.ballman added inline comments.Feb 17 2022, 11:19 AM
clang/lib/AST/ASTContext.cpp
8555–8556

I don't insist on the change; I mostly found it odd to have an uninitialized local that is initialized on the very next line.

This revision was landed with ongoing or failed builds.Feb 17 2022, 11:34 AM
This revision was automatically updated to reflect the committed changes.
pcc marked an inline comment as done.Feb 17 2022, 11:38 AM