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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly looks good.



================
Comment at: llvm/lib/LTO/LTO.cpp:809
+    } else
+      llvm_unreachable("unknown symbol type");
+
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Use braces for the `else` block to keep it uniform with the then block.


================
Comment at: llvm/lib/LTO/LTO.cpp:832
+  // block.
+  if (!M.getModuleInlineAsm().empty()) {
+    std::string NewIA = ".lto_discard";
----------------
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?


================
Comment at: llvm/test/MC/ELF/lto-discard.s:9
+
+.lto_discard foo
+.weak foo
----------------
There needs to be a test checking that `.lto_discard` without arguments works.
You can use `--defsym` to place multiple tests in one file if keeping them in one file can help readers.
See some existing `--defsym` tests for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98762