[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

[Sami Tolvanen via Phabricator via cfe-commits]

samitolvanen added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:77
+      // references from inline assembly.
+      std::string Alias = ".set \"" + OldName + "\",\"" + NewName + "\"\n";
+      ExportM.appendModuleInlineAsm(Alias);
----------------
rnk wrote:
> samitolvanen wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > Is there more information about "promotion aliases with x86_64-pc-windows-msvc" from D106392? Can we quote these only for msvc target triples? Can we add a comment about the quoting be necessary for those targets?
> > > > 
> > > > It's still not clear to me how tests using explicit -linux-gnu triples could fail on -mscv hosts.
> > > Also, I think the quoting hurts the readability of the generated asm.  Maybe that doesn't matter for LTO, but I'd be curious if we could do such escaping only when necessary? Perhaps that's only when targeting -msvc triples?
> > > Is there more information about "promotion aliases with x86_64-pc-windows-msvc" from D106392?
> > 
> > Yes, the -msvc targets use Visual C++ compatible name mangling, which requires quotes when referred to in inline assembly. Here's a trivial reproducer:
> > 
> > ```
> > $ cat test.cpp 
> > static void a(void) {}
> > void* b(void) { return (void *)a; }
> > $ clang -flto=thin -fvisibility=default -fsanitize=cfi -c test.cpp
> > $ clang -flto=thin -fvisibility=default -fsanitize=cfi -target x86_64-pc-windows-msvc -c test.cpp
> > Either SourceMgr should be available
> > UNREACHABLE executed at llvm-project/llvm/lib/MC/MCContext.cpp:913!
> > ...
> > ```
> > 
> > Here's the inline assembly generated when we compile the above example for the -msvc target:
> > ```
> > .set ?a@@YAXXZ,?a@@YAXXZ.d7b56b39ccc53bc7515ae1b2533f1e3d
> > ```
> > 
> > > Can we quote these only for msvc target triples? Can we add a comment about the quoting be necessary for those targets?
> > 
> > I assume we could limit this only to -msvc targets, but I feel like that would unnecessarily complicate the code as there's no harm in quoting the names always.
> > 
> > > It's still not clear to me how tests using explicit -linux-gnu triples could fail on -mscv hosts.
> > 
> > These tests don't specify a `-linux-gnu` triple. They are C++ and end up using the default target, which in this case is `-msvc`.
> I was going to say, the proper way to do this is what `MCSymbol::print` does:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCSymbol.cpp#L59
> 
> That code doesn't seem exhaustive, but at least it escapes quotes. We can't call MC from here due to library layering.
> 
> The LLVM IR readability is poor, but inline asm in IR is already hard to read. I wouldn't worry about that, I'd mainly worry about the output of -S.
> 
> Quoting always seems fine to me, I guess.
> I was going to say, the proper way to do this is what `MCSymbol::print` does:
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCSymbol.cpp#L59
> 
> That code doesn't seem exhaustive, but at least it escapes quotes. We can't call MC from here due to library layering.

Is it actually possible to have function names that contain quotes? If so, I suppose we need to do something similar here and escape any quotes in the names.

> The LLVM IR readability is poor, but inline asm in IR is already hard to read. I wouldn't worry about that, I'd mainly worry about the output of -S.

The promotion happens only when we write bitcode, so the aliases won't yet exist in the `-S` output.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104058/new/

https://reviews.llvm.org/D104058