Currently, StackMapParser only supports dumping stack map of version 3.
We should warn user when dumping stack map of other versions.
This partly fixes https://bugs.llvm.org/show_bug.cgi?id=38278
|11 ↗||(On Diff #219269)|
Please remove the excessive indentations.
|16 ↗||(On Diff #219269)|
Doesn't seem you need AddressAlign or Flags fields?
|17 ↗||(On Diff #219269)|
Would be nice to have a comment about how you got this content.
Variable name should be uppercase.
I think James will suggest a different wording, but anyways I think we should include the version found to the error message.
You should use reportWarning from llvm-readobj.h
|1 ↗||(On Diff #219269)|
Please add a comment to the top of this test briefly outlining its purpose.
|2 ↗||(On Diff #219269)|
We try to use double-dashes for options in new llvm-readobj tests these days, as single-dash causes problems with option grouping.
Perhaps unrelated, but please get rid of this blank line.
Warnings (and errors) should be lower-case first letter and no trailing full stop or new line, if using reportWarning as @grimar suggests.
Additionally, I'd recommend rewording as:
"stack map entry with version <version> detected at offset <offset>. Only version 3 is supported"
where <version> is the read in version and <offset> is the hex offset of the start of the stack map entry within the section. I'm assuming that there can be multiple entries in a single section, in a linked object (if so, please test this message for multiple such entries).
Addressed @grimar 's comments
Addressed @jhenderson 's comments
supported -> supports
Maybe this should return an array of versions, and be called "getSupportedVersions"? Obviously in the first case, the array has only one member, but it would make it easy to add more in the future.
|1–2 ↗||(On Diff #219474)|
I'd change the last bit of this comment to:
"... reports a warning when parsing an unsupported stack map version."
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.
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.
I googled about stackmaps, and it's possible to have multiple entries of stackmap. Currently, llvm-readobj assumes there is only one entry per file. See this thread: https://bugs.llvm.org/show_bug.cgi?id=38277
I've tested on my machine, using trunk version of clang. If both object files have one stackmap section, linker just concatenates them into one section. So, this is a bug, right? I would like to hard-code the offset as 0x0 in this patch, and improve this behavior in the future. Do you happy with my opinion?
llvm-readobj's behaviour is a bug. The linker concatenating them I think is fine behaviour (it's what it does for e.g. .debug_* sections).
I would like to hard-code the offset as 0x0 in this patch, and improve this behavior in the future. Do you happy with my opinion?
I'd suggest pulling the offset into a local variable, so that you can put a FIXME comment, referencing the bug you mentioned, explaining why the variable is there. That means that the message won't need updating in the future. On the other hand, just a FIXME by the message for now is fine too.
You can get rid of the ':' after "See" and add a trailing full stop.
I'd rephrase the first sentence as: "There could be multiple entries in a .llvm_stackmaps section, but this code only handles the first." The version issue isn't really the whole story here, so there's no need to specifically highlight it.
I think the usual pattern is /*LowerCase=*/true (where LowerCase is the parameter name).
The trailing full stop should be after the hyperlink, not after the "See":
(Actually, I see that Phabricator tries to add the '.' to the hyperlink (as well as the double quotes...). Not sure if this is specific to Phabricator or if other editors make this mistake too, so you might just want to get rid of the full stop altogether in this case).
Please use the exact formatting as in my previous comment, not as you have done. It's important to get right, because clang-format recognises this pattern (i.e. without the spaces).
It says you check llvm-readelf, but you don't. You might want to add it too.
It is probably not that clear that "not supported" is not about "at offset 0x0", but about version?
Perhaps, the following is slightly better:
You also do not need a full stop in the message.