[PATCH] D76999: [ELF] Print symbols with non-default versions for better "undefined symbol" diagnostics

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay 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();
----------------
grimar wrote:
> grimar wrote:
> > 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 :)
> This my concern perhaps should not be a stopper for this patch, since you have LGTM.
I have thought about the problem, but it is difficult to improve the situation.
nameSize is truncated here:

```lang=cpp
void Symbol::parseSymbolVersion() {
  StringRef s = getName();
  size_t pos = s.find('@');
  if (pos == 0 || pos == StringRef::npos)
    return;
  StringRef verstr = s.substr(pos + 1);
  if (verstr.empty())
    return;

  // Truncate the symbol name so that it doesn't include the version string.
  nameSize = pos;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76999