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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/Offloading/failure.test:17
+
+# CHECK: Invalid data was encountered while parsing the file
----------------
jhenderson wrote:
> This doesn't tell us whether the message is a warning or error, so please include the "warning:" or "error:" prefix in the check.
> 
> Additionally, the error message as checked doesn't give sufficient context as to what has gone wrong. For example, it doesn't mention the input file or that it is the offloading code that has failed. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages. A couple of options include modifying the underlying code to give better error messages, or to "catch" the error and rewrap it with some additional context in the message. llvm-readobj has a number of examples of where this is done that I know of, for example (llvm-objdump may do too).
> 
> Finally, in dumping tools, especially in newer code, we try to avoid having hard errors. The reason for this is because it is often useful to be able to see the reainder of the output from other files and/or options, whereas reporting an error is usually a hard end to the program (NB: I haven't double-checked the llvm-objdump behaviour to confirm that the error reporting ends the program, so apologies if this is a bit of misdirection).
> 
> I made the second and third of these points in previous inline comments, but they don't seem to have been addressed.
`ReportError` does indeed exit the program. There are a lot of other examples of `llvm-objdump` exiting on malformed input. I think it's reasonable to exit if the user requested `--offloading` and it's malformed to just exit. I will specify the checks and try to improve the message however.


================
Comment at: llvm/tools/llvm-objdump/OffloadDump.cpp:35-36
+    return "ptx";
+  default:
+    return "<None>";
+  }
----------------
jhenderson wrote:
> jhenderson wrote:
> > jhuber6 wrote:
> > > jhenderson wrote:
> > > > jhenderson wrote:
> > > > > I think it would be sensible to test the default case (it's a common behaviour pattern for the default case to result in an error, so it seems sensible to demonstrate that it isn't in this case).
> > > > This comment was marked as done, but I don't see a test case for it?
> > > Because I added some validation to `obj2yaml` it became impossible to test it by creating one without a valid value, but I'm going to get rid of that to simplify this and add it back in.
> > As a general rule, yaml2obj should be as lax as possible in what it allows, as it allows testing these corner cases.
> With the changes made to yaml2obj, can we now test this default case?
It's already being tested now. The `binary.yaml` file has the fourth entry with the `None` type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126904