rsmith requested changes to this revision.
rsmith added a comment.
This revision now requires changes to proceed.
A lot of the test changes here seem to be over-constraining the existing tests.
Tests that don't care about `nonnull` / `dereferenceable` `this` pointers
should generally not be checking for that. Instead of looking for `nonnull
dereferenceable(...)`, I'd prefer to see `{{[^,]*}}` or similar to skip any
parameter attributes.
I would also like to see some tests that directly cover the new functionality:
specifically, tests that check that the argument to `dereferenceable` is
correct, including cases such as classes with virtual bases, virtual function
calls, calls through pointers to member functions, and a check that we don't
emit the attributes under `-fno-delete-null-pointer-checks`.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2174
+ }
+
unsigned ArgNo = 0;
----------------
CJ-Johnson wrote:
> jdoerfert wrote:
> > Even if null pointer is valid we should place dereferenceable.
> >
> > We also could never place nonnull and let the middle-end make the
> > dereferenceable -> nonnull deduction, though I don't see why we can't just
> > add nonnull here.
> I re-ran ninja check after making this fix and addressed the newly-affected
> tests. So the patch is fully up to date :)
The LLVM LangRef says that in address space 0, `dereferenceable` implies
`nonnull`, so I don't think we can emit `dereferenceable` in
`NullPointerIsValid` mode, and we'd need to use `dereferenceable_or_null`
instead. (Perhaps the LangRef is wrong, though, and the `null_pointer_is_valid`
function attribute overrides that determination.)
================
Comment at: clang/test/CodeGenCXX/array-default-argument.cpp:22-24
+ // CHECK: {{call|invoke}} {{.*}} @_ZN1BC1E1A([[TEMPORARY:.*]])
// CHECK-EH: unwind label %[[A_AND_PARTIAL_ARRAY_LPAD:.*]]
+ // CHECK: {{call|invoke}} {{.*}} @_ZN1AD1Ev([[TEMPORARY:.*]])
----------------
This has changed the meaning of the test. Previously we were checking the
argument bound in the first call is also passed to the second and third calls;
now we're not checking the call arguments at all, because we re-bind
`TEMPORARY` in each `CHECK` line.
================
Comment at:
clang/test/CodeGenCXX/microsoft-abi-multiple-nonvirtual-inheritance.cpp:126
//
-// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* %[[RIGHT]])
+// CHECK: call x86_thiscallcc void %[[VFUN_VALUE]](i8* nonnull %[[RIGHT]])
// CHECK: ret
----------------
Why does this call get `nonnull` but not `dereferenceable`? Seems like we
should be able to use at least `dereferenceable(sizeof(Right))` here -- but I
think we could actually be more aggressive and pass
`dereferenceable(sizeof(ChildOverride) - offsetof(ChildOverride, <Right base
class>))`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D17993/new/
https://reviews.llvm.org/D17993
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
