Page MenuHomePhabricator

Handle ConstantExpr correctly in SelectionDAGBuilder

Authored by Praetonus on Jun 22 2017, 3:00 PM.



This change fixes a bug in SelectionDAGBuilder::visitInsertValue and SelectionDAGBuilder::visitExtractValue where constant expressions (InsertValueConstantExpr and ExtractValueConstantExpr) would be treated as non-constant instructions (InsertValueInst and ExtractValueInst). This bug resulted in an incorrect memory access, which manifested as an assertion failure in SDValue::SDValue.

Fixes PR#33094.

Diff Detail


Event Timeline

Praetonus created this revision.Jun 22 2017, 3:00 PM
efriedma edited reviewers, added: efriedma; removed: llvm-commits.Jun 22 2017, 3:25 PM
efriedma added subscribers: llvm-commits, efriedma.

Missing testcase... although, I'm not how you would write one.

Could we fix SelectionDAGBuilder::visit so this problem can't happen again?

3228 ↗(On Diff #103643)

"else", instead of "else if"?

davide added a subscriber: davide.Jun 22 2017, 3:30 PM

I didn't read this fix so I can't say for sure but there's a reduced test in the original PR
so if that doesn't crash anymore, it should be better than no test at all.

@efriedma Sorry for the test case, I did write one based on the reduced case from the PR, I'm not sure why SVN didn't include it in the diff. Anyway, I'll update to include it.

I'm not sure how SelectionDAGBuilder::visit could be fixed. It seems to me that instruction opcodes are shared by both the constant and non-constant version of an instruction, so the function wouldn't be able to handle a constant and a non-constant instruction separately unless it uses isa or a similar mechanism. I could be missing something though since my knowledge of the LLVM code base is rather limited.

Praetonus updated this revision to Diff 103662.Jun 22 2017, 4:26 PM

A test case has been added and the patch has been updated based on @efriedma's comments.

efriedma edited edge metadata.Jun 23 2017, 12:53 PM

I'll think about the SelectionDAGBuilder::visit issue myself, and maybe come up with a followup patch.

2 ↗(On Diff #103662)

Needs a comment describing what it's testing.

Writing tests without any CHECK lines is usually a bad idea, because they tend to be very easy to break, but I'm not sure what we would want to check for here, exactly, so I guess this is okay.

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
2 ↗(On Diff #103662)

Shouldn't this at least include a REQUIRES: asserts?

efriedma added inline comments.Jun 23 2017, 1:43 PM
2 ↗(On Diff #103662)

Why would we want "REQUIRES: asserts"? The test isn't checking debug output, or anything like that.

Praetonus updated this revision to Diff 103966.Jun 26 2017, 8:51 AM

I've added a comment describing the test case.

This revision is now accepted and ready to land.Jun 26 2017, 12:40 PM

Sorry, I forgot to mention that I don't have commit access. Could somebody commit this on my behalf?

This revision was automatically updated to reflect the committed changes.