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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
