This is an archive of the discontinued LLVM Phabricator instance.

Do not emit prologue_end for line 0 locs if there is a non-zero loc present
ClosedPublic

Authored by rastogishubham on Sep 29 2021, 10:47 AM.

Details

Summary

This change fixes a bug where the compiler generates a prologue_end for line 0 locs. That is because line 0 is not associated with any source location, so there should not be a prolgoue_end at a location that doesn't correspond to a source location.

There were some LLVM tests that were explicitly checking for line 0 prologue_end's as well since I believe that to be incorrect, I had to change those tests as well.

Diff Detail

Event Timeline

rastogishubham requested review of this revision.Sep 29 2021, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 10:47 AM
probinson added inline comments.
llvm/test/CodeGen/X86/line-zero-prologue-end.ll
15

This seems like an excessive amount of metadata for a test that cares only about line numbers. I assume you generated this from some Swift example; could you compile it with -gline-tables-only instead of just -g to reduce the fluff?

llvm/test/DebugInfo/MIR/X86/debug-loc-0.mir
8

This change looks like it causes the test to stop verifying what the comment on line 1 says: collapse line-0 directives. The changed test would pass if we emitted

.loc 1 0 0
.loc 1 0 0
.loc 1 37 1 prologue_end

which is not what this test wants.

rastogishubham added inline comments.Sep 29 2021, 2:18 PM
llvm/test/CodeGen/X86/line-zero-prologue-end.ll
15

Thanks for the feedback Paul, yes this is a swift example, I will write a small test.c file and try to get a smaller testcase. Thanks

llvm/test/DebugInfo/MIR/X86/debug-loc-0.mir
8

I see what my error is here, so I assume this test is trying to see that multiple line 0 locs should be collapsed into one. So all I need to do is something like.

# CHECK: Ltmp0:
# CHECK: .loc 1 0 0
# CHECK-NOT: .loc 1 0 0
rastogishubham added inline comments.Sep 29 2021, 2:32 PM
llvm/test/DebugInfo/MIR/X86/debug-loc-0.mir
8

I meant:

# CHECK: Ltmp0:
# CHECK: .loc 1 0 0
# CHECK-NOT: .loc 1 0 0
# CHECK: .loc 1 37 1 prologue_end

As suggested by Adrian and Paul, I have reduced the original testcase to be smaller by using a small C sample and compiling only with -gline-tables-only. I have also added another test case where if the only .loc is a line 0 loc, we want to have a prologue_end there, and only move the prologue_end to a non-zero .loc if it exists.

I have also edited my change to llvm/test/DebugInfo/MIR/X86/debug-loc-0.mir to be consistent with what it is testing

rastogishubham retitled this revision from Do not emit prologue_end for line 0 locs to Do not emit prologue_end for line 0 locs if there is a non-zero loc present.Sep 29 2021, 6:43 PM

Fixed the clang-tidy warnings

It may feel like I'm being a bit picky, but the prologue_end business is a bit picky, and it's helpful to have the tests be as clear as possible about what's expected.

llvm/test/CodeGen/X86/line-zero-prologue-end.ll
5

Should this have prologue_end on it?

8

Should this check that prologue_end is *not* on it?

26

Please make !9 and !14 use different line numbers, to make it more obvious which one is the source for the .loc directive.

Thanks, this LGTM after addressing all of @probinson s comments!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2097

Maybe we should also explain the rationale behind this:

// Scan forward to try to find a non-zero line number. The prologue_end marks the first breakpoint in the function after the frame setup, and a compiler-generated line 0 location is not a meaningful breakpoint. If none is found, return the first location after the frame setup.
2100

This is also a problem with the original code, but findPrologueEndLoc should probably not scan past the first basic block. Your patch doesn't make this any worse though.

rastogishubham marked 4 inline comments as done.Oct 1 2021, 11:54 AM
rastogishubham added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2097

Completely agree, thanks for that comments

llvm/test/CodeGen/X86/line-zero-prologue-end.ll
5

This should *not* have a prologue_end on it, I will add a regex check for the end of the line to make sure prologue_end doesn't show up

8

This also should not have a prologue_end on it, I will add a regex check for it too.

26

Sure, I will just change it to line 3

rastogishubham marked 4 inline comments as done.

Updated the test files make sure there are check's for where prologue_end's should not be present. Added a better comment to explain the change made to findPrologueEndLoc as suggested

Added a regex check for ensuring no prologue_end is there on no-non-zero-debug-loc-prologue.ll

@probinson Hi Paul, is it possible for you to take another look? I have updated the test files to be more strict with the prologue_ends.

Thanks,

Shubham

probinson accepted this revision.Oct 7 2021, 11:40 AM

Thanks for the ping, I hadn't added this to my to-do list. LGTM

This revision is now accepted and ready to land.Oct 7 2021, 11:40 AM