[PATCH] D67328: [llvm-readobj] Warn user when dumping not supported version of stack map (PR38278).

Tue Sep 10 01:57:18 PDT 2019

jhenderson added inline comments.

================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:29-30
+                      "stack map entry with version " + Twine(Version) +
+                      " detected at offset " + Twine(0) + ". Only version " +
+                      Twine(SupportedVersion) + " is supported"),
+                  Filename);
----------------
grimar wrote:
> MaskRay wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > If you accept my suggestion of returning multiple supported versions, you'll need to change this message again (sorry!). I'd suggest changing the second sentence to "Supported version(s): " and then simply list the array members.
> > > I think just "version X is not supported" should be fine. Is it important to report a list of supported versions here? I am not sure.
> > `"... offset" + utohexstr(Offset)`
> > 
> > Offsets are usually represented as a hex number. Since the offset is always 0, you can just hard code `"offset 0x0"`
> >  Since the offset is always 0, you can just hard code "offset 0x0"
> 
> Why not to omit the offset at all from the message then?
Does the linker not glue multiple stack_map entries together into a single section? The offset comment was intended to be if there were multiple such entries in a single section. The first could be version 3, but later ones other versions, and should be warned for. If that's not possible, then the offset shouldn't be printed.

I admit, I haven't looked at the stack map code elsewhere in enough depth to know if any of this makes sense. The llvm-readobj code clearly only supports a single entry.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67328