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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

OK, I think this makes sense and likely fixes https://github.com/ClangBuiltLinux/linux/issues/1269.

A STB_GLOBAL definition can override a STB_WEAK definition. However, if we stream them in the order `(.local foo) foo: (foo is not used, see parseAssignmentExpression) .weak foo; foo: ...`

The binding will change to STB_WEAK, while we should actually respect the prevailing definition's binding: STB_LOCAL.

If the symbol is used by regular object files, we may get a `changed binding` error.

It is unfortunate that we don't very elegant way nuking non-prevailing inline asm definitions.

Please fix the few suggestions.



================
Comment at: llvm/lib/LTO/LTO.cpp:733
+  std::string Sep = getTargetAsmSeparator(M);
+  std::string PrefixPat = "(^|[[:blank:]]+)";
+  std::string SuffixPat = "($|" + Sep + ")";
----------------
StringRef


================
Comment at: llvm/lib/LTO/LTO.cpp:735
+  std::string SuffixPat = "($|" + Sep + ")";
+  std::string Symbols = "(" + join(NonPrevailingAsmSymbol, "|") + ")";
+  std::vector<std::string> Pats = {// Remove assignment directive.
----------------
llvm::join


================
Comment at: llvm/lib/LTO/LTO.cpp:745
+                                       Symbols + "[[:blank:]]*" + SuffixPat};
+  for (auto P : Pats) {
+    Regex RE(PrefixPat + P, Regex::Newline);
----------------
auto causes an unnecessary std::string copy.


================
Comment at: llvm/lib/LTO/LTO.cpp:751
+
+  if (std::all_of(IA.begin(), IA.end(), [](auto c) { return std::isspace(c); }))
+    IA = "";
----------------
This is not needed. Just let MC ignore whitespace.


================
Comment at: llvm/test/LTO/X86/weak-asm.ll:6
+; RUN: llvm-lto2 run -o %t4 -save-temps %t1 %t2 %t3 \
+; RUN:  -r %t1,foo,px \
+; RUN:  -r %t2,foo, \
----------------
%t1 + %t2 is sufficient and makes more sense to the linker (there is no duplicate definition error).

You can add another test using %t2 + %t3.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96931