The logic employed by the constant folder assumes the GEPs to be inbounds, when applied to other GEPs the result can be potentially incorrect and cause miscompilations.
Details
Diff Detail
Event Timeline
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
1837 | Are non-inbounds GEP common enough to implement this? |
@nlopes Can you please take a look at the alive result? I don't think the transform is correct, but alive says it is. The result of the GEP is a pointer of value zero. It has different provenance from the null pointer, but icmp eq should be a pure value comparison, independent of provenance, right?
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
1837 | I'd generally say that we should handle inbounds precisely just for the sake of clarity -- however, after reviewing all the folds below, it looks like all of them either require inbounds, or can only work on GEPs for which we would infer inbounds anyway (e.g. the gep GV1, 0 icmp gep GV2, 0 case). Given that, the early out here should be fine, and I think you can drop the TODO as well. | |
1847–1850 | Unrelated to this patch, but all the isSigned handling in this code looks wrong to me. This is effectively saying that GlobalValues can only appear in the lower half of the address space and not in the upper (negative) half, which is not true. |
I don't have a clear model for the semantics of pointer comparison ATM; Pointer comparison *sometimes* needs to take provenance into consideration because LLVM folds p1 == p2 where p1 and p2 are pointing to two different zero-size objects having the same address into false.
Also, considering provenance into account allows aggressively folding pointer comparisons. It isn't clear how *frequently* the provenance should be considered.
But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.
I don't have a clear model for the semantics of pointer comparison ATM; Pointer comparison *sometimes* needs to take provenance into consideration because LLVM folds p1 == p2 where p1 and p2 are pointing to two different zero-size objects having the same address into false.
Also, considering provenance into account allows aggressively folding pointer comparisons. It isn't clear how *frequently* the provenance should be considered.
FWIW, these are a few examples and relevant links:
- A ptr comparison folding to false regardless of addresses of two possibly overlapping stack allocations: https://godbolt.org/z/ok4vFJ , bugzilla report: https://bugs.llvm.org/show_bug.cgi?id=44342
- Another example: https://godbolt.org/z/bhrsK1 (well it wasn't a zero-size object, but simply merging two identical global objects. Anyway it shows that defining ptr comparison is something non-trivial), reduced from: https://bugs.llvm.org/show_bug.cgi?id=47090 , https://github.com/rust-lang/rust/issues/73722
I'm not sure why zig generates this code, but the context here is https://github.com/ziglang/zig/issues/6408. The issue was exposed by D93820, as we previously simply folded away the GEP to null, losing provenance.
My understanding was that the current resolution on pointer comparison issues is that pointer comparison is value-only, and provenance-losing equality replacements in GVN are what we consider incorrect. But I haven't followed all the discussions.
The original case here was actually not "gep == null", but "ptrtoint gep == 0", in which case it's obviously a pure value comparison, but InstSimplify looks through that. Do you think that's the real issue?
But, I'm rather curious about how the miscompilation happened from this optimization. A gep with such offset isn't common, unless a programmer writes a code that subtracts a pointer from null (which is already fishy)?
It would be great if I can see the input that causes miscompilation.
As stated here the gep is being generated by the SCEV pass.
A minimal test case that shows the gep being created (but not this miscompilation) is here.
llvm/lib/IR/ConstantFold.cpp | ||
---|---|---|
1837 | Some trivial geps can still be folded such as all-zero ones vs constant or unsigned comparisons with zero. But yes, I'll drop the TODO as those cases are pretty rare. | |
1847–1850 | Yes, that looks wrong. I guess this is unlikely to cause problems as most (every?) pointer comparisons against null are of eq/neq type. |
Resigning here, based on past experience trying to challenge the Word Of God. It really doesn't help that someone opened an alive issue for this without cross-referencing it, so the same discussion is repeated twice.
Actually this is indeed a problematic transformation; you said a very valid point.
Removing it has large impact (it leaves ptrtoint to an object which is conservatively considered to escape the object), so simply removing the transformation is hard ATM, but I believe it should be removed at some point.
It is bad that there were two discussions with the same topic; maybe the link was not shared here because the Alive2's side did not have a very clear solution about ptr comparison as well. If I could answer anything clear about the pointer comparison, the link must have been shared :(
+1, Fixing SCEV seems to be the easiest solution at this point.
Alive pointer icmp semantics have recently been fixed and now agree that this is a miscompile, so I think we should move forward with this patch. It will need a rebase as some additional tests have been added in the meantime. This should also fix https://bugs.llvm.org/show_bug.cgi?id=50208.
llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll | ||
---|---|---|
203 | For the tests that changed, could you please add a copy that has the inbounds keyword, so that the folding case is still covered? |
llvm/test/Transforms/InstSimplify/ConstProp/icmp-null.ll | ||
---|---|---|
203 | There's something weird going on, SimplifyGEPInst in InstructionSimplify.cpp is silently dropping the inbounds flag thus preventing the fold from happening. |
Since we settled on pointer comparison being equivalent to address comparison (i.e., provenance is not taken into account), then the current code is correct.
See table 2 here, for example: https://web.ist.utl.pt/nuno.lopes/pubs/alive2-mem-cav21.pdf#page=10
Therefore I suggest we drop this patch.
The optimization that is wrong under this model is one in InstSimplify that folds pointer comparison between pointers of different objects to false. That is not correct for geps that aren't inbounds.
Are non-inbounds GEP common enough to implement this?