[PATCH] D126904: [llvm-objdump] Add support for dumping embedded offloading data

Fri Jul 1 04:19:01 PDT 2022

jhuber6 added a comment.

In D126904#3624373 <https://reviews.llvm.org/D126904#3624373>, @jhenderson wrote:

> In D126904#3622100 <https://reviews.llvm.org/D126904#3622100>, @jhuber6 wrote:
>
>> Addressing comments. I think what we need is some new function like `reportErrorNoExit`, otherwise all the file paths handling the printing themselves would be ugly.
>
> This is definitely something I think the tool could benefit from, with a final check at the end of the program to ensure the right exit code is produced, if this function has ever been called. LLD has a similar process for non-fatal errors (it sets a variable that is checked periodically to ensure it is safe to continue). That being said, as this impacts llvm-objdump more widely, I would be happy for it to be deferred to a later patch, so that other code paths could be updated at the same time.
>
> There are a high number of inline comments that I haven't seen be addressed. Most of them are fairly minor, but the volume of them combined with the fact that I've had no response to them makes me concerned that letting the patch land without them means they'll never be addressed. Please could you go through them and either address them or explain why it should either a) be deferred or b) not done at all.

The only comments I'm aware of that I haven't addressed is early exiting on failure and a check on failing to get the section. I think if we truly want to avoid early exits we should put it in a separate patch, I'm not doing anything that the rest of `llvm-objdump` doesn't already do. I do not think it's necessary to add an error check if we fail to get a section, this is tested already in many places and I see no point to add a completely redundant test to the LLVM test-suite. I can remove the quotation marks prior to landing if you want. If you have any further objections let me know, otherwise I would greatly appreciate being able to move forward with this.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904