Page MenuHomePhabricator

[llvm-symbolizer] Fix native symbolization on windows for inline sites.
ClosedPublic

Authored by akhuang on Nov 25 2020, 1:31 PM.

Details

Summary

The existing code handles this correctly and I checked that the code
in NativeInlineSiteSymbol also handles this correctly, but it was
wrong in the NativeFunctionSymbol code.

Diff Detail

Event Timeline

akhuang created this revision.Nov 25 2020, 1:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2020, 1:31 PM
akhuang requested review of this revision.Nov 25 2020, 1:31 PM

I haven't added any tests here; I could upload an msvc-built binary for testing this. Not sure if that's necessary?

rnk added a comment.Nov 25 2020, 4:00 PM

Well, I *can* think of a way to test this from a .s file. Instead of using .cv_inline_line_table, you could use a sequence of .byte 0xNN, 0xNN directives. I think you can get the hex codes from llvm-pdbutil dump -symbols. How about it?

llvm/lib/DebugInfo/PDB/Native/NativeFunctionSymbol.cpp
81

I see. LLVM never uses this opcode. If this opcode is used to create a gap in the inlined call site, CodeOffset will be incorrect after the gap.

In D92134#2417347, @rnk wrote:

Well, I *can* think of a way to test this from a .s file. Instead of using .cv_inline_line_table, you could use a sequence of .byte 0xNN, 0xNN directives. I think you can get the hex codes from llvm-pdbutil dump -symbols. How about it?

How does this work? If I just add .bytes they turn into instructions. Or does it need to start with a particular .byte sequence?

rnk added a comment.Nov 30 2020, 10:53 AM
In D92134#2417347, @rnk wrote:

Well, I *can* think of a way to test this from a .s file. Instead of using .cv_inline_line_table, you could use a sequence of .byte 0xNN, 0xNN directives. I think you can get the hex codes from llvm-pdbutil dump -symbols. How about it?

How does this work? If I just add .bytes they turn into instructions. Or does it need to start with a particular .byte sequence?

If you set up a regular S_INLINESITE record in codeview, and then replace the .cv_inline_line_table directive with raw bytes, you can more finely control the opcodes used to produce the binary annotations. You'd get rid of the associated .cv_loc directives for the inline site as well.

akhuang updated this revision to Diff 308465.Nov 30 2020, 12:59 PM

Add test case

rnk accepted this revision.Nov 30 2020, 1:18 PM

lgtm, thanks!

This revision is now accepted and ready to land.Nov 30 2020, 1:18 PM
This revision was landed with ongoing or failed builds.Nov 30 2020, 2:32 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Nov 30 2020, 3:07 PM

This seems to break tests everywhere, eg http://45.33.8.238/linux/34060/step_11.txt

sorry, I messed up the code when rebasing; should be fixed now

Yes, cycled green. Thanks for the quick fix!