Page MenuHomePhabricator

[llvm-readobj] Warn user when dumping not supported version of stack map (PR38278).
Needs ReviewPublic

Authored by Higuoxing on Sep 8 2019, 8:29 AM.

Details

Reviewers
jhenderson
grimar
Summary

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

Event Timeline

Higuoxing created this revision.Sep 8 2019, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2019, 8:29 AM
MaskRay added a subscriber: MaskRay.Sep 8 2019, 9:05 AM
grimar added inline comments.Sep 9 2019, 1:44 AM
llvm/test/tools/llvm-readobj/elf-stackmap-version2.test
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.
However, it seems it can be just Content: 02 for now?

llvm/tools/llvm-readobj/StackMapPrinter.h
29

Variable name should be uppercase.

31

I think James will suggest a different wording, but anyways I think we should include the version found to the error message.

32

You should use reportWarning from llvm-readobj.h

jhenderson added inline comments.Sep 9 2019, 4:04 AM
llvm/test/tools/llvm-readobj/elf-stackmap-version2.test
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.

llvm/tools/llvm-readobj/StackMapPrinter.h
28

Perhaps unrelated, but please get rid of this blank line.

31

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).

Higuoxing updated this revision to Diff 219473.Sep 9 2019, 8:39 PM

Addressed @grimar 's comments

  • Remove excessive indentions.
  • Remove some unused fields (Flags, AddressAlign).
  • Minimize contents of stack map section. (Only keep version in header field).
  • Change variable name to upper case.
  • Use reportWarning.

Addressed @jhenderson 's comments

jhenderson added inline comments.Sep 10 2019, 1:25 AM
llvm/include/llvm/Object/StackMapParser.h
325–332

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.

llvm/test/tools/llvm-readobj/elf-stackmap-version2.test
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."

llvm/tools/llvm-readobj/StackMapPrinter.h
32–33

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.

grimar added inline comments.Sep 10 2019, 1:28 AM
llvm/include/llvm/Object/StackMapParser.h
325–332

I'd refactor it to bool isVersionSupported(unsigned Ver) then. Returning an array perhaps a bit overcomplicated thing for a public API.

grimar added inline comments.Sep 10 2019, 1:30 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
32–33

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.

MaskRay added inline comments.Sep 10 2019, 1:44 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
32–33

"... offset" + utohexstr(Offset)

Offsets are usually represented as a hex number. Since the offset is always 0, you can just hard code "offset 0x0"

grimar added inline comments.Sep 10 2019, 1:47 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
32–33
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?

jhenderson added inline comments.Sep 10 2019, 1:55 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
32–33

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.

Higuoxing updated this revision to Diff 219524.EditedSep 10 2019, 5:16 AM

Thank you @grimar, @jhenderson and @MaskRay .

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?

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?

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.

Higuoxing updated this revision to Diff 219552.Sep 10 2019, 8:49 AM

Yes, I mean this is a bug of llvm-readobj. Sorry for my unclear expression.

Is this FIXME hint ok?

jhenderson added inline comments.Sep 10 2019, 9:09 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
25

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.

33

I think the usual pattern is /*LowerCase=*/true (where LowerCase is the parameter name).

Higuoxing updated this revision to Diff 219562.Sep 10 2019, 9:38 AM

Addressed @jhenderson 's comments.

jhenderson added inline comments.Sep 11 2019, 2:17 AM
llvm/tools/llvm-readobj/StackMapPrinter.h
25

The trailing full stop should be after the hyperlink, not after the "See":

"See https://bugs.llvm.org/show_bug.cgi?id=38277."

(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).

33

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).

Higuoxing updated this revision to Diff 219675.Sep 11 2019, 2:35 AM

Sorry, I see some patterns like /* LowerCase */ true elsewhere, and have no idea that this is not a proper pattern.

Thanks a lot for pointing this out :)

Higuoxing updated this revision to Diff 220418.Sep 16 2019, 6:39 PM

Rebase & Run tests.

Hi, @jhenderson, @grimar and @MaskRay, are you happy happy with this revision now?

grimar added inline comments.Sep 17 2019, 1:22 AM
llvm/test/tools/llvm-readobj/elf-stackmap-unsupported-version-2.test
1

It says you check llvm-readelf, but you don't. You might want to add it too.

6

stack map entry with version 2 detected at offset 0x0, which is not supported.

It is probably not that clear that "not supported" is not about "at offset 0x0", but about version?

Perhaps, the following is slightly better:
stack map entry with unsupported version 2 detected at offset 0x0

You also do not need a full stop in the message.
(Having it is inconsistent with the other ones we have in this tools I believe).

Looks good, once @grimar's comments have been addressed.