This is an archive of the discontinued LLVM Phabricator instance.

[MS] Mangle a hash of the main file path into anonymous namespaces
ClosedPublic

Authored by rnk on Aug 16 2018, 5:16 PM.

Details

Summary

This is needed to avoid conflicts in mangled names for codeview types in
anonymous namespaces. In CodeView, types refer to each other typically
through forward declarations, which contain mangled names. These names
have to be unique, otherwise the debugger will look up the mangled name
and find the wrong definition.

Furthermore, ThinLTO will deduplicate the types, and debug info
verification can fail when the types have the wrong sizes. This is
PR38608.

Fixes PR38609.

Diff Detail

Repository
rC Clang

Event Timeline

rnk created this revision.Aug 16 2018, 5:16 PM

How does MSVC handle this case? What mangled name does it generate?

IIRC it’s ?A0xABCDABCD@ where the hex value is some kind of hash

inglorion added inline comments.Aug 16 2018, 5:48 PM
clang/lib/AST/MicrosoftMangle.cpp
389 ↗(On Diff #161147)

Instead of MD5, can we use xxhash (or whatever the fastest hash algorithm in LLVM is these days)? I don't think we need a particular hash algorithm, so we might as well go with the fastest one we have (as long as it does a good job avoiding collisions, of course).

rnk updated this revision to Diff 161293.Aug 17 2018, 11:34 AM
  • Use xxHash64
rnk marked an inline comment as done.Aug 17 2018, 11:38 AM

Exactly, this makes our names match MSVC more closely. Their hash depends on the path to the main source file. It doesn't care if the file is in a header. However, they use the absolute path to the file instead of the (probably relative) path to the file passed by the build system on the command line. I think my approach of using the spelling of the path on the command line will play better with distributed build systems, since you can imagine multiple distributed workers compiling at different absolute paths.

clang/lib/AST/MicrosoftMangle.cpp
389 ↗(On Diff #161147)

Sure. It's not visible in the ABI, so we're free to change it at any point later.

thakis accepted this revision.Aug 17 2018, 12:50 PM
thakis added a subscriber: thakis.

Can you explicitly mention that this intentionally doesn't use an absolute path in MicrosoftMangleContextImpl() or similar?

This revision is now accepted and ready to land.Aug 17 2018, 12:50 PM
rnk updated this revision to Diff 161310.Aug 17 2018, 1:23 PM
rnk marked an inline comment as done.
  • improve comment
rnk added a comment.Aug 17 2018, 1:52 PM

Can you explicitly mention that this intentionally doesn't use an absolute path in MicrosoftMangleContextImpl() or similar?

Sure. I also described the issue with codeview that motivates why we want unique names.

I think I sent this out hastily at the end of the day yesterday, and I didn't write enough comments to answer all of your questions. Hopefully with these changes it makes more sense.

This revision was automatically updated to reflect the committed changes.