Skip to content

C++: AssignPointerAddExpr and AssignPointerSubExpr should not be bitwise operations #14635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

MathiasVP
Copy link
Contributor

Apparently we've been considering AssignPointerAddExpr and AssignPointerSubExpr as bitwise operations for almost 4 years (since I introduced this bug in one of my very first GitHub good-first-issues: 11a545e 🙈)

@github-actions github-actions bot added the C++ label Oct 30, 2023
@MathiasVP MathiasVP marked this pull request as ready for review October 31, 2023 09:53
@MathiasVP MathiasVP requested a review from a team as a code owner October 31, 2023 09:53
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a library change note for this?

@jketema
Copy link
Contributor

jketema commented Oct 31, 2023

I think it would also make sense to run a DCA experiment with uninterpreted queries enabled.

@MathiasVP
Copy link
Contributor Author

Should we add a library change note for this?

Yeah, I think that's a good idea.

I think it would also make sense to run a DCA experiment with uninterpreted queries enabled.

Doh! Good point. I'll rerun DCA with uninterpreted queries enabled.

@jketema
Copy link
Contributor

jketema commented Oct 31, 2023

You might want to add data set check for full measure

@MathiasVP
Copy link
Contributor Author

@jketema the DCA run with dataset checks have completed now. I think it LGTM. Can I get your 👀 on it as well?

@jketema
Copy link
Contributor

jketema commented Oct 31, 2023

3 hours seems suspiciously fast, and I'm missing the frontend timings for some reason.

@MathiasVP
Copy link
Contributor Author

3 hours seems suspiciously fast, and I'm missing the frontend timings for some reason.

Oh wait. Does dataset measure require me to disable database caching? 🤔

@jketema
Copy link
Contributor

jketema commented Oct 31, 2023

You made a schema change, I you will need to run without caching.

@MathiasVP
Copy link
Contributor Author

MathiasVP commented Oct 31, 2023

You made a schema change, I you will need to run without caching.

That's ... a good point. Starting another one without DB caching now, then!

@jketema
Copy link
Contributor

jketema commented Oct 31, 2023

Also, it's a bit suspect that we get a change in IR inconsistencies with caching enabled. That means the schema change somehow affects IR generation?

@MathiasVP
Copy link
Contributor Author

Also, it's a bit suspect that we get a change in IR inconsistencies with caching enabled. That means the schema change somehow affects IR generation?

Yeah, I was wondering about that as well. I wrote it off as noise, but let me just investigate this while waiting for this (hopefully) last DCA run anyway.

@MathiasVP
Copy link
Contributor Author

Can't reproduce the IR inconsistencies locally. Running the "count IR inconsistencies" query locally on the cached openzfs DB (on the HEAD of this PR) yields:

missingOperand unexpectedOperand duplicateOperand missingPhiOperand missingOperandType duplicateChiOperand sideEffectWithoutPrimary instructionWithoutSuccessor ambiguousSuccessors unexplainedLoop unnecessaryPhiInstruction memoryOperandDefinitionIsUnmodeled operandAcrossFunctions containsLoopOfForwardEdges lostReachability backEdgeCountMismatch useNotDominatedByDefinition switchInstructionWithoutDefaultEdge notMarkedAsConflated wronglyMarkedAsConflated invalidOverlap nonUniqueEnclosingIRFunction fieldAddressOnNonPointer thisArgumentIsNonPointer nonUniqueIRVariable
520 0 0 1 3,726 0 0 104 2,970 0 2 0 0 0 0 0 59,391 0 44 0 0 0 0 0 77,431

which is exactly what we have on the "before" side on DCA.

@MathiasVP
Copy link
Contributor Author

DCA looks fine. The same IR inconsistencies are still present, but that may now be from nondeterministic extraction.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, still needs a change note though.

@MathiasVP
Copy link
Contributor Author

LGTM, still needs a change note though.

Added in 6e385ca.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants