This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] LLVM ERROR: Broken function found, while removing Debug Intrinsics
ClosedPublic

Authored by CarlosAlbertoEnciso on Jan 30 2019, 3:37 AM.

Details

Summary

This patch includes a fix for a compilation failure, introduced by:

https://reviews.llvm.org/D53287
https://reviews.llvm.org/rL345250

The test case that causes the compilation failure, was supplied by Mikael Holmen.

For the given test case, the verifier complains with the error message:

LLVM ERROR: Broken function found, compilation aborted!

The issue has been reported as:

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

Diff Detail

Repository
rL LLVM

Event Timeline

The fix makes sense to me but please let others have an opinion too.

Thanks!

test/CodeGen/X86/bbi-23595.ll
11 ↗(On Diff #184266)

Perhaps add some positive CHECKs instead that verifies that we get the optimized output that we want?
We only get a single bb with the phi and ret left I think?

LGTM with a positive CHECK.

Thanks very much for your invaluable feedback and reviews.

Updated patch to include positive CHECKs on the optimized output.

CarlosAlbertoEnciso marked an inline comment as done.Jan 31 2019, 2:26 AM
CarlosAlbertoEnciso added inline comments.
test/CodeGen/X86/bbi-23595.ll
11 ↗(On Diff #184266)

Uploaded a patch to include positive CHECKs.

dstenb added a subscriber: dstenb.EditedJan 31 2019, 2:32 AM

Maybe it can be good to rename the test file to either the PR number, or something that says what it verifies? The "bbi-" number refer to one of Ericsson's internal issue trackers.

(Although, I guess that we wouldn't mind if the name was kept as-is :-))

dstenb added inline comments.Jan 31 2019, 2:53 AM
test/CodeGen/X86/bbi-23595.ll
14 ↗(On Diff #184473)

It seems to me that the negative check is unnecessary now. The positive checks verify that the llvm.dbg.label is dropped. Also, if the verifier errors out, opt exits with a non-zero exit code, so the test would fail due to that.

CarlosAlbertoEnciso marked an inline comment as done.Jan 31 2019, 5:15 AM
CarlosAlbertoEnciso added inline comments.
test/CodeGen/X86/bbi-23595.ll
14 ↗(On Diff #184473)

You are correct. The negative check is not required. I tried the test case without the fix and the it fails.

Updated patch to remove the negative CHECK. Thanks for your feedback.

aprantl added inline comments.Jan 31 2019, 4:08 PM
test/CodeGen/X86/bbi-23595.ll
12 ↗(On Diff #184487)

I would put all this meta-info in the commit message and instead focus on describing what is being tested here. Something like: We're expecting the dbg.label to disappear because ...

Updated patch to address comments from @aprantl .

Thanks for the feedback.

CarlosAlbertoEnciso marked an inline comment as done.Feb 7 2019, 5:45 AM
CarlosAlbertoEnciso added inline comments.
test/CodeGen/X86/bbi-23595.ll
12 ↗(On Diff #184487)

I have updated the patch following your comments and I will use the standard short message in the commit.

I will include in the bugzilla, the following text:

Note: This patch is a complement to pr38762.

For the given test case, the verifier complains with the error message:

LLVM ERROR: Broken function found, compilation aborted!
The issue was introduced at https://reviews.llvm.org/rL345250.
aprantl accepted this revision.Feb 7 2019, 10:28 AM
This revision is now accepted and ready to land.Feb 7 2019, 10:28 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 2:57 AM

Thanks @aprantl, @uabelho for your valuable review.

dstenb added a comment.Feb 8 2019, 4:00 AM

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

As this review is closed and committed, I will create another revision just to move the test to the 'Transforms' directory. Thanks

Sorry for not noticing this before, but we noticed now that the test case is placed under test/CodeGen/X86/, although it is a simplifycfg test. Should it ideally be placed under test/Transforms/SimplifyCFG/?

As this review is closed and committed, I will create another revision just to move the test to the 'Transforms' directory. Thanks

Don't bother with a review, just do it.

@aprantl, as per your feedback I have moved the test case to the correct location.