[PATCH] D65313: [llvm-readelf] Dump the stack sizes sections with --stack-sizes

[Wolfgang Pieb via Phabricator via llvm-commits]

wolfgangp added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/stack-sizes.test:194
+
+## Check that we report an error when a stack sizes section ends with a complete stack size entry.
+
----------------
jhenderson wrote:
> "with a complete" sounds like there's a mistake here. Should it be "within"?
I've been struggling with the phrasing of this from the beginning. I wanted to say "ends with an incomplete stack size entry".


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4403
+    } else if (*SectionOrErr != FunctionSec) {
+      reportWarning(" '" + FileStr + "': relocation symbol " + SymName +
+                    " is not in the expected section");
----------------
grimar wrote:
> 3 comments/suggestions:
> 
> 1) Instead of manually constructing the message with a file name,
>     you can use the same approach as already used now for reporting a errors.
> i.e. just introduce a method:
> 
> ```
> void reportWarning(StringRef Input, Error Err) {
>   if (Input == "-")
>     Input = "<stdin>";
>   warn(createFileError(Input, std::move(Err)));
> }
> ```
> (see my D65515 for reference).
> 
> 2) With that you do not need to call `consumeError` after `reportWarning`.
> 
> 3) I think everywhere where you used `consumeError` you should probably report a warning.
> Like in the code above:
> 
> ```
>     if (NameOrErr)
>       SymName = *NameOrErr;
>     else
>       consumeError(NameOrErr.takeError());
> ```
> i.e. call `reportWarning` from (1) instead.
> 
> What do you think?
I like the idea of adapting the reporting of warnings to the one with errors.

However, I feel this should really be done in a different patch, and then we can refactor all uses of reportWarning in ELFDumper.cpp and ObjDumper.cpp. I'd be happy to do this afterwards, while it's still fresh in my mind.

The other caveat is that with reporting warnings on all malformed input we would now create messages on, for example, malformed symbols that have nothing to do with what we are reporting on (since we're scanning the symbol table). If you feel that's acceptable, I have no problem doing it, but I wanted to point it out.




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

https://reviews.llvm.org/D65313