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

[Sami Tolvanen via Phabricator via llvm-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);
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104058