This is an archive of the discontinued LLVM Phabricator instance.

AST: Create __va_list in the std namespace even in C.
ClosedPublic

Authored by pcc on Jun 23 2021, 6:21 PM.

Details

Summary

This ensures that the mangled type names match between C and C++,
which is significant when using -fsanitize=cfi-icall. Ideally we
wouldn't have created this namespace at all, but it's now part of
the ABI (e.g. in mangled names), so we can't change it.

Diff Detail

Event Timeline

pcc requested review of this revision.Jun 23 2021, 6:21 PM
pcc created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 6:21 PM
pcc updated this revision to Diff 354130.Jun 23 2021, 6:24 PM

Add test

This revision was not accepted when it landed; it landed in state Needs Review.Jun 23 2021, 6:59 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
jrtc27 added a subscriber: jrtc27.EditedNov 13 2021, 6:27 PM

This results in the DWARF type info containing DW_TAG_namespace for C, which breaks DTrace's CTF (as the name implies, it is for C, not C++), as used during the build of FreeBSD. This does not seem correct to me. If your mangling for va_list is broken then you should special case that in the CFI mangler as an ABI quirk, IMO, not change the DWARF for C to contain C++ things.

Or you can still do it in the frontend, just make sure that the DWARF emitted by the frontend doesn't have the DINamespace wrapper when not compiling C++.

I also note that 32-bit Arm uses the same namespacing, and has CFI-icall support (at least, it's enabled, I don't know if it's actually expected to work) but did not have this change made to it (though probably best not to do so until the DWARF issue is resolved)

This also leaks out to users via __builtin_dump_struct (https://godbolt.org/z/vx3rjdPdq), and of course the DWARF having the namespace in it will result in users debugging plain C seeing C++ namespaces.

ASTDiagnostic's Desugar happens to special-case va_list so it doesn't get desugared from the result of the typedef, and thus the struct std::__va_list doesn't leak out that way.

I also wouldn't be surprised if some consumers of Clang's AST get confused by a namespace appearing for C output; e.g. I could imagine IDEs using clangd displaying very confusing information to users writing C code.

pcc added a comment.Nov 20 2021, 5:54 PM

Agreed that this should probably be done in the mangler, I'll try to take a look next week.

Great, thanks

Any luck with this?

pcc added a comment.Jan 6 2022, 3:37 PM

Ping?

Sorry for the delay, D116774 should fix this.