[PATCH] D56453: Modify InputSectionBase::getLocation to add section and offset to every location string.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu accepted this revision.
ruiu added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: ELF/InputSection.cpp:310
   if (Defined *D = getEnclosingFunction<ELFT>(Offset))
-    return SrcFile + ":(function " + toString(*D) + ")";
+    return SrcFile + ":(function " + toString(*D) + ": " + SecAndOffset + ")";
 
----------------
sfertile wrote:
> ruiu wrote:
> > I think that the idea here is that a function name and an object file name uniquely identifies an error location. A section name and an offset is the last resort if there's no other information we can display.
> > 
> > Did you find it useful to print out a section name and an offset in addition to function name? If so, in what situation?
> > 
> The motivation for this change comes from a range-check error message I get from a compiler generated file. Since the compiler adds the filename we get a  message that look roughly like this:
> ` test.c:(function  foo): relocation R_PPC64_ADDR16_DS out of range: 32808 is not in [-32768, 32767]` 
> 
> The problem is that foo could potentially have dozens of `R_PPC64_TOC16_DS` relocations. By adding the section and offset I can pinpoint exactly which relocation is overflowing/underflowing immediately. The same benefits holds for the `checkAlignment` diagnostics.
> 
> I agree for some of the error messages the extra info holds no value. For example the diagnostic for split-stack prologue adjust failure is completely characterized by the file and the function. I think I would prefer to keep the extra info even in this case so that the diagnostics are consistent and we don't need to be able to generate 2 slightly different formattings but I don't feel to strongly either way.
> 
> 
I'm fine with extra info as long as you find that is useful. At least that shouldn't hurt anyone. Thank you for the explanation!


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D56453