vsk added a comment.
Thanks for the comments, responses inline --
================
Comment at: lib/CodeGen/CGExprScalar.cpp:3854
+ const Twine &Name) {
+ Value *GEPVal = Builder.CreateInBoundsGEP(Ptr, IdxList, Name);
+
----------------
filcab wrote:
> You're creating the GEP first (possibly triggering UB), and then checking
> ("after" UB). Shouldn't you put the checking instructions before the GEP?
The checking instructions are not users of the (possibly poisoned) GEP value so
inserting the checks after the GEP should be OK. FWIW ubsan's scalar range
checks do the same thing and I haven't seen an issue there.
I do have a concern about the IR constant folder reducing GEPs to Undef, but I
don't think this affects any interesting cases, and we handle that below by
bailing out if the GEP is a Constant.
Does that clear up your concerns?
================
Comment at: lib/CodeGen/CGExprScalar.cpp:3948
+ return GEPVal;
+
+ // Now that we've computed the total offset, add it to the base pointer (with
----------------
filcab wrote:
> Do we want an extra test for `TotalOffset` being a constant + not
> overflowing? (Not strictly need, but you've been working on avoiding
> __ubsan() calls :-) )
If the total offset is a constant and doesn't overflow, there's no need for the
select instruction below, but we'd still need to emit a pointer overflow check.
Teaching clang to omit the select is a little messy, and I don't think we'd
save very much, so I'm leaning against doing it.
https://reviews.llvm.org/D33305
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
