[PATCH] D98762: [LTO][MC] Discard non-prevailing defined symbols in module-level assembly

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/LTO/LTO.cpp:832
+  // block.
+  if (!M.getModuleInlineAsm().empty()) {
+    std::string NewIA = ".lto_discard";
----------------
ychen wrote:
> MaskRay wrote:
> > If the previous `.lto_discard` directive is not empty, you'll need to reset the list to empty, otherwise can this discard some prevailing symbols?
> `NonPrevailingAsmSymbols` is empty initially for each module and the module without inlineasm block is not considered. I think it should be fine?
I am thinking whether this scenario may happen and drop the prevailing foo accidentally.

```
.lto_discard foo
non-prevailing foo
prevailing foo
```

Perhaps this is impossible.


================
Comment at: llvm/lib/LTO/LTO.cpp:757
   std::set<const Comdat *> NonPrevailingComdats;
+  SmallSet<StringRef, 4> NonPrevailingAsmSymbols;
   for (const InputFile::Symbol &Sym : Syms) {
----------------
Use 2 to be consistent with `SmallSet<StringRef, 2> LTODiscardSymbols;`


================
Comment at: llvm/lib/LTO/LTO.cpp:836
+    if (!NonPrevailingAsmSymbols.empty()) {
+      // Don't dicard a symbol if there is a live .symver for it.
+      ModuleSymbolTable::CollectAsmSymvers(
----------------
`Don't dicard a symbol if there is a live .symver for it.` better to have a test.


================
Comment at: llvm/test/MC/ELF/lto-discard.s:3
+// .lto_discard is supposed to only work for inline assembly.
+// RUN: llvm-mc -filetype=asm -triple x86_64-pc-linux-gnu < %s | FileCheck %s
+
----------------
`-filetype=asm` does not have effects. You need a `-filetype=obj` test to show that the `foo` definition is ignored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98762