[PATCH] D37013: [ELF] - Add additional information about location when emiting linkerscript's parsing errors.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/ScriptLexer.cpp:81-87
+  if (!Context.empty())
+    error(Context);
+
   if (!Pos) {
     error(getCurrentLocation() + ": " + Msg);
     return;
   }
----------------
In general, do not call `error` multiple times for an error message. Call `error` only once per an error instead. This is important because we have the -error-limit option which manages the number of errors (and that is different from number of lines of error messages.)

I think you can now see why our error message format is something like `cannot parse file: /foo/bar`. It's because you can concatenate multiple error messages by joining with ":" like this:

  error while reading linker script foo.ls: cannot parse file: /foo/bar

So, concatenate error strings into one string and print it out using `error` just like above.


================
Comment at: ELF/ScriptLexer.h:45
   size_t Pos = 0;
+  std::string Context;
 
----------------
grimar wrote:
> peter.smith wrote:
> > It might be better to call this LocationContext if it is specifically going to be used within setLocationContext() and dropLocationContex(). This might help prevent it being used/overwritten in other non LocationContext situations by mistake. 
> May be, or I was thinking about following names now:
> `ErrorContext`, `setErrorContext`, `dropErrorContext`
I'd name ErrorPrefix as this string is printed as a prefix for an error message.


================
Comment at: ELF/ScriptParser.cpp:607
 OutputSection *ScriptParser::readOutputSectionDescription(StringRef OutSec) {
+  setLocationContext("unable to parse description for output section '" +
+                     OutSec + "':");
----------------
grimar wrote:
> peter.smith wrote:
> > peter.smith wrote:
> > > Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?
> > Would it be worth using "section definition for output section" rather than "description for output section" to match the Linker Script manual?
> > Is it worth an RAII wrapper so that you don't have to explicitly remember to call dropLocationContext()?
> 
> I tried 2 more ways here:
> * Simple class that sets/drops context in constructor/destructor.
> * Storing `std::weak_ptr<std::string> Context`. That way we can do something like:
> 
> ```
> std::shared_ptr<std::string> setErrorContext(const Twine& Msg) {
>   auto Ret = make_shared<std::string>(Msg.cstr());
>   Context = Ret;
>   return Ret;
> }
> ...
> auto C = setErrorContext("blah blah blah");
> <and while this shared pointer is alive, following code will print context>
> ...
> 
> if (auto C = Context.lock())
>   error(*C);
> 
> ```
> 
> But I am not sure it worth to do now, when there is only single place where context is used.
> 
> > Would it be worth using "section definition for output section" rather than "description for output section" to match the > Linker Script manual?
> 
> I'll change it, thanks. My wording was based on method name here.
RAII is a good idea, but if you don't want to do that, you don't want to create a set of functions just to set `Context` local varaible. You could just directly assign values to the variable.


https://reviews.llvm.org/D37013