This is an archive of the discontinued LLVM Phabricator instance.

[CodeView] Fix inline sites that are missing code offsets.
ClosedPublic

Authored by akhuang on Dec 3 2020, 10:55 AM.

Details

Summary

When an inline site has a starting code offset of 0, we sometimes
don't emit the starting offset.

This patch removes the case that emits a BinaryAnnotationOpCode::ChangeLineOffset when the code offset is 0. These cases will just go to the next case that emits a BinaryAnnotationOpCode::ChangeCodeOffsetAndLineOffset.

Bug: https://bugs.llvm.org/show_bug.cgi?id=48377

Diff Detail

Event Timeline

akhuang created this revision.Dec 3 2020, 10:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 10:55 AM
akhuang requested review of this revision.Dec 3 2020, 10:55 AM
akhuang edited the summary of this revision. (Show Details)Dec 3 2020, 11:08 AM
dblaikie added inline comments.
llvm/test/DebugInfo/COFF/inline-site-syms.ll
8–13 ↗(On Diff #309308)

I'd probably suggest a simpler (it's probably about the same in IR, but simpler in terms of language features, names, etc) test case for inlining, something like:

void f1();
__attribute__((always_inline)) void f2() {
  f1();
}
void f3() {
  f2();
}

If that's adequate.

rnk added a comment.Dec 3 2020, 1:25 PM

Is this a bug in llvm-symbolizer? What does windbg think if you stop on the first instruction of such an inline call site?

Looks like other test expectations need to be updated as well.

Oh, I do need to fix the existing tests. Actually, the test I wrote probably belongs in that test directory.

I don't think it's a bug in llvm-symbolizer since I don't think we can assume that an inline site starts at offset 0 if it's not specified. llvm-symbolizer with DIA also fails to find the inline site here; I haven't looked at what windbg does.

akhuang added inline comments.Dec 4 2020, 10:53 AM
llvm/test/DebugInfo/COFF/inline-site-syms.ll
8–13 ↗(On Diff #309308)

yep, I mostly used this particular code because the offset of the inline site in the function happens to be 0. But I suppose this should actually have been an assembly test and changing the existing tests should be adequate test coverage for this--

akhuang updated this revision to Diff 309581.Dec 4 2020, 10:53 AM

Update failing tests

rnk accepted this revision.Dec 7 2020, 12:33 PM

lgtm

This revision is now accepted and ready to land.Dec 7 2020, 12:33 PM
This revision was automatically updated to reflect the committed changes.