On 2/22/21 8:00 PM, Martin Sebor wrote:
On 2/22/21 4:08 PM, Jason Merrill wrote:
On 2/13/21 7:31 PM, Martin Sebor wrote:
The test case in PR 99074 invokes dynamic_cast with the this pointer
in a non-static member function called on a null pointer. The call
is, of course, undefined and other different circumstances would be
diagnosed by -Wnonnull. Unfortunately, in the test case, the null
pointer is the result of inlining and constant propagation and so
detected neither by the front end -Wnonnull nor by the middle end.
The program ends up passing it to __dynamic_cast() which then
crashes at runtime (again, not surprising for undefined behavior.
However, the reporter says the code behaved gracefully (didn't crash)
when compiled with GCC 4.8, and in my tests it also doesn't crash
when compiled with Clang or ICC. I looked to see if it's possible
to do better and it seems it is.
The attached patch improves things by changing __dynamic_cast to
fail by returning null when the first argument is null, and also
This hunk is OK.
by declaring __dynamic_cast with attribute nonnull so that invalid
calls to it with a constant null pointer can be detected at compile
time.
This is not; dynamic_cast is specified to return null for a null operand.
"If v is a null pointer value, the result is a null pointer value."
The undefined behavior is the call to _to_object, not the dynamic_cast.
Yes, of course. Just to be clear, in case it's not from the patch,
it adds nonnull to the __dynamic_cast() function in libsupc++ which
(if I read the comment right) documents nonnull-ness as its
precondition. The function should never be called with a null
pointer except in buggy code like in the PR.
True. So the attribute is technically correct, but the resulting
warning is misleading, since the user shouldn't need to know anything
about the internal implementation of dynamic_cast, and the user-visible
feature doesn't have that constraint.
I don't think this
is the most elegant way to diagnose the user bug but I also couldn't
think of anything better. Do you have any suggestions? (I ask in
part because for GCC 12 I'd like to see about issuing the warning
requested on PR 12277.)
If we can manage to warn about a null argument to __dynamic_cast, I'd
think we should also be able to warn about a null 'this' argument to
_to_object.
WRT to the documentation of __dynamic_cast(), I didn't remove
the comment in the patch that mentions the precondition because
of the new warning. If we want to consider null pointers valid
input to the function it seems we should update the comment. Do
you agree?
The comment is from the ABI
(https://itanium-cxx-abi.github.io/cxx-abi/abi.html#dynamic_cast-algorithm),
we shouldn't change it.
The comment is below. I assume SUB refers to SRC_PTR.
/* sub: source address to be adjusted; nonnull, and since the
* source object is polymorphic, *(void**)sub is a virtual pointer.
* src: static type of the source object.
* dst: destination type (the "T" in "dynamic_cast<T>(v)").
* src2dst_offset: a static hint about the location of the
* source subobject with respect to the complete object;
* special negative values are:
* -1: no hint
* -2: src is not a public base of dst
* -3: src is a multiple public base type but never a
* virtual base type
* otherwise, the src type is a unique public nonvirtual
* base type of dst at offset src2dst_offset from the
* origin of dst. */
extern "C" void *
__dynamic_cast (const void *src_ptr, // object started from
const __class_type_info *src_type, // type of the
starting object
const __class_type_info *dst_type, // desired target type
ptrdiff_t src2dst) // how src and dst are related
Since the test case is undefined it seems borderline whether this
can strictly be considered a regression, even if some previous
releases handled it more gracefully.
Indeed. But handling the null case in __dynamic_cast as well as in
the compiler seems harmless enough.
Okay.
I guess it's a question of which behavior is preferable in (this) case
of undefined behavior. Doing something reasonable is in general a valid
choice. OTOH, the crash revealed the undefined behavior in the testcase.
Jason
