[PATCH] D27900: [ELF] - Keep the source file/line location information separate from the object file location information.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Error.cpp:84
 
+void elf::hint(const Twine &Msg) {
+  std::lock_guard<std::mutex> Lock(Mu);
----------------
ruiu wrote:
> If you add `hint` after `warn` in the header, please add to the same place in .cpp.
Done.


================
Comment at: ELF/Error.h:40
 void warn(const Twine &Msg);
 
+void hint(const Twine &Msg);
----------------
ruiu wrote:
> Remove this blank line.
Done, though that was done intentionally initially. "hint" is not a error and not a log/warning,
but I have no major preference here to keep it separate from first group too.

Also I renamed "hint" to "note". As it prints "note: ....".
Actually I am thinking about this as about "hint", that why named in that way. But consistency with clang probably matters here.


================
Comment at: ELF/InputSection.cpp:42
 template <class ELFT>
+void elf::errorAtOffset(const InputSectionBase<ELFT> &IS,
+                        const llvm::Twine &Msg, typename ELFT::uint Off) {
----------------
ruiu wrote:
> I don't think this is a good addition because we already have too many error() wrapper functions. Aggregating two function calls into one doesn't help us that much, and I think it actually decreases readability. Please write it in two functions.
I am also would not do that change. I see no problems in having one more wrapper. It incapsulates some logic and avoids code duplication.
Sometimes we have a function for single call, like "convertPathToUnix" or alike for readability and that is fine.
Actually problem that I see is that them are thown everywhere around in code but not in a local file intended for error reporting. I would suggest to make error reporting to be as much local as possible and just move that to error.cpp.

So, I did that change as well, but I do not feel it was good one.




================
Comment at: ELF/InputSection.cpp:225
 template <class ELFT>
-std::string InputSectionBase<ELFT>::getLocation(typename ELFT::uint Offset) {
+static std::string getExtendedName(const InputSectionBase<ELFT> *IS,
+                                   typename ELFT::uint Offset) {
----------------
ruiu wrote:
> This is another `Ext` and that should be avoided.
Done.


================
Comment at: ELF/InputSection.h:147
+  // error message.
+  std::string getLocationExt(uintX_t Offset) const;
 
----------------
ruiu wrote:
> Please avoid `Ext` suffix because it doesn't convey meaning (with that naming scheme we'll end up having something like getDataExtExt.) You need to name variables/functions based on what they are instead of whether they are new or old.
I renamed it back. But I would like to explane why I did that (actually your mail about communication issues about LLD had a good effect for me, so I probably will want sometimes to explain why I did the changes instead of just replying "Done" as I did before).

Initially during development of this patch I had two methods. They were getLocationBase (it was inlined finally) and this one, getLocationExt.
I thought about name of last one. "getLocationForOffset", "getExtendedLocation", "getLocationHint". "getLocation" just was not ideal naming for me, because it did not reveal this function used for note/hint to provide additional info uncovered by getLocationBase.
Now I do not have "getLocationBase", but that does not mean purpose of "getLocationExt" changed, it still returns something more than a regular location.





================
Comment at: ELF/SymbolTable.cpp:372
   print(NewLoc + ": duplicate symbol '" + toString(*Existing) + "'");
-  print(OldLoc + ": previous definition was here");
+  std::string NewLocHint = D->Section->getLocationExt(D->Value);
+  hint("definition found in " + NewLocHint);
----------------
ruiu wrote:
> Why do you need this, `OldLoc` and `OldLocHint`? You can just write in place.
Done. I feel my way was a bit more readable though.


================
Comment at: ELF/SymbolTable.cpp:376
+  std::string OldLoc = toString(D->Section->getFile());
+  hint(OldLoc + ": previous definition was here");
+  std::string OldLocHint = ErrSec->getLocationExt(ErrOffset);
----------------
ruiu wrote:
> This results in an odd message.
> 
>   note: foo.o: previous definition was here
> 
> This should be
> 
>   note: previous definition was here: foo.o
> 
Done.


================
Comment at: ELF/Target.cpp:58
 
+static void errorAtLocation(const uint8_t *Loc, const llvm::Twine &E) {
+  std::pair<std::string, std::string> Msg = getErrorLocation(Loc);
----------------
ruiu wrote:
> Please avoid writing a new wrapper.
Done. But I do not understand why. New code looks a bit bulky and overduplicated after doing that change.


================
Comment at: ELF/Target.cpp:64
+
+static void fatalAtLocation(const uint8_t *Loc, const llvm::Twine &E) {
+  errorAtLocation(Loc, E);
----------------
grimar wrote:
> ruiu wrote:
> > Please avoid writing a new wrapper function.
> > 
> > What is this function? I think we don't want to print out "fatal error encountered" at end.
> Target.cpp now sometimes fatal(), for example when reloc is unrecognized.
> So actually problem is to call some error message + hint(). I can't just call
> ```
> fatal(...).
> hint(...)
> ```
> because user will not see hint.
> 
> That is why I introduced this function. Though I think no need to worry about it probably.
> I believe what we can do at first - is to change fatal() to errors() in a separate patch. That removes
> need of such hack. I'll try to prepare a patch, not sure why we fatal() now.
So patch for this was D27973. I replaced calls with error() instead of fatal() here assuming it be landed first.

But please look at EhReader::failOn(). I still have to call fatal() there.
I think I can prepare a patch to avoid fatal() in EhReader, what do you think ?


https://reviews.llvm.org/D27900