[PATCH] D67182: [lib/ObjectYAML] - Improve and cleanup error reporting in ELFState<ELFT> class.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for working on this. I think you need a few more tests to show that we no longer `exit(1)` where we previously were (i.e. that we can report multiple error messages). I see you have one for relocation references, but there are several other places that need testing too.



================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:274
+  else
+    reportError("unknown section referenced: '" + S + "' at YAML section '" +
+                LocSec + "'");
----------------
at -> by


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:285
+  if (!SymMap.lookup(S, Index) && !to_integer(S, Index)) {
+    reportError("unknown symbol referenced: '" + S + "' at YAML section '" +
+                LocSec + "'");
----------------
at -> by


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:416
 
-  return true;
+  return !HasError;
 }
----------------
Maybe you could have this function return void, and then have the caller check `HasErrors`?


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:453
-                           << "' by YAML symbol " << Sym.Name << ".\n";
-        exit(1);
-      }
----------------
Adding a test case to show that we can report multiple errors for this issue would be good.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:589
+template <class ELFT> void ELFState<ELFT>::reportError(const Twine &Msg) {
+  WithColor::error() << Msg << "\n";
+  HasError = true;
----------------
This might be a bit of a bigger task than you had hoped for, and I'm okay if you want to push this suggestion to a later change, but it would be great if this could call some form of error handler passed in by the client of the library instead of directly calling `WithColor::error()`. That way the client can control how the errors are reported. See my lightning talk on error handling in libraries from the US conference a couple of years ago.

Regardless, setting `HasError` here seems fine, and obviously a default option of `WithColor::error()` is sensible.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:606
+                    "' by program header");
+        return;
       }
----------------
Maybe this could be a `continue` instead of a `return`. That way all unknown sections will be picked up.


================
Comment at: lib/ObjectYAML/ELFEmitter.cpp:718-719
-    unsigned SymIdx = 0;
-    // If a relocation references a symbol, try to look one up in the symbol
-    // table. If it is not there, treat the value as a symbol index.
-    if (Rel.Symbol && !SymMap.lookup(*Rel.Symbol, SymIdx) &&
----------------
Maybe it's worth having something like this comment in `toSymbolIndex`?


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

https://reviews.llvm.org/D67182