[PATCH] D25826: [ELF] Show error location for 'undefined symbol' errors

[Rui Ueyama via llvm-commits]

ruiu added a comment.

Thank you for doing this. This would be very useful.



================
Comment at: ELF/Error.cpp:42
+void elf::error(const Twine &Source, const Twine &Msg) {
+  *ErrorOS << Source << ": error: " << Msg << "\n";
   HasError = true;
----------------
I think I'm inclined to showing ARGV[0] even for undefined symbol errors to keep them consistent with other linker-issued errors. I do not see a reason to not do except for making error lines a bit shorter.


================
Comment at: ELF/InputFiles.cpp:405
 
 template <class ELFT>
+DefinedRegular<ELFT> *
----------------
A brief comment would be appreciated. Please note that this is not an optimized function but it is okay because this is for creating an error message.


================
Comment at: ELF/InputFiles.cpp:407
+DefinedRegular<ELFT> *
+elf::ObjectFile<ELFT>::getEnclosingSymbol(InputSectionBase<ELFT> *S,
+                                          uintX_t Offset) {
----------------
This function can be implemented as a non-member function, no? Non-member functions are preferred over member functions if they access only public members.


================
Comment at: ELF/InputFiles.cpp:409-411
+  for (SymbolBody *B : SymbolBodies) {
+    auto *D = dyn_cast<DefinedRegular<ELFT>>(B);
+    if (!D)
----------------
  for (...)
     if (auto *D = ...)
       if (D->Value <= ...)
          return D;
  return nullptr;


================
Comment at: ELF/InputFiles.h:178-179
 
+  // Returns name of source file or empty string, if it cannot be fetched.
+  StringRef getSourceFile() { return SourceFile; }
+
----------------
I'd make `SourceFile` a public member variable and remove the accessor, as the accessor doesn't contain a logic.


================
Comment at: ELF/InputFiles.h:182
+  // Gets symbol which encloses given offset and belong to given section
+  DefinedRegular<ELFT> *getEnclosingSymbol(InputSectionBase<ELFT> *S,
+                                           uintX_t Offset);
----------------
Probably a personal taste, but how about `getSymbolAt`?


https://reviews.llvm.org/D25826