[PATCH] D76999: [ELF] Print versioned name for better "undefined symbol" diagnostics

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Symbols.cpp:41
+  // part. This check is safe because every symbol name ends with '\0'.
+  if (name.data()[name.size()] == '@')
+    ret += name.data() + name.size();
----------------
Though it is technically looks correct at this moment, it looks a bit hacky still. It violates the contract `StringRef` provides:
it is unexpected that user will access the buffer outside of its `Size`.

Is it a time to make `toString` method a member of `Symbol`? Since it is a global function that now relies on the assumption about internal implementation. (With that you will be able to access `nameData` member directly, what should be cleaner).

I also wonder what other people think. May be it just me who think it is a bit too hacky :)


================
Comment at: lld/ELF/Symbols.cpp:45
+}
 std::string toELFString(const Archive::Symbol &b) {
   return demangle(b.getName());
----------------
Missing an empty line before this one.


================
Comment at: lld/ELF/Symbols.h:24
 namespace lld {
+// Print the versioned name, optionally demangled. This function is used by
+// diagnostics for better context.
----------------
"Print"? Perhaps "Returns" as it does not actually pring anything to an output.


================
Comment at: lld/test/ELF/undef-suggest-version.s:6
+
+## Suggest @@ for an unversioned reference.
+# RUN: echo 'call bat' | llvm-mc -filetype=obj -triple=x86_64 - -o %tdef1.o
----------------
I'd rephrase. It probably reads like we expect to see "did you mean: bar@@v1` in the error message?
Perhaps: "Check we suggest the symbol with a default version..."


================
Comment at: lld/test/ELF/undef-suggest-version.s:38
+
+# RUN: echo '.symver foo.v2,foo at v2; call foo.v2' | llvm-mc -filetype=obj -triple=x86_64 - -o %thidden2.o
+# RUN: not ld.lld %t.so %thidden2.o -o /dev/null 2>&1 | FileCheck --check-prefix=HIDDEN2 %s
----------------
MIssing a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76999