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

Fri Jul 1 00:45:00 PDT 2022

jhenderson added a comment.

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.

================
Comment at: llvm/test/tools/llvm-objdump/Offloading/failure.test:17
+
+# CHECK: error: 'invalid data encountered while extracting offloading files'
----------------
jhenderson wrote:
> Unrelated to my other comments, but why is this string in single quotes?
To be clear, I don't think the explanation needs to be in quotes of any variety - only the file name.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904