On Fri Jul 10, 2026 at 11:16 PM WEST, David Malcolm wrote:
>> Some notes on devirt-multiple-inheritance-1.C:
>>
>> Virtual calls through a non-primary base (e.g. B::g on C : A, B via a
>> B*) go through a this-adjusting thunk (_ZThn8_...).
>> get_fndecl_for_virtual_call resolves the thunk to the underlying method
>> via function_symbol, so can_throw_p sees the right nothrow status.
>>
>> However, the thunk's fixed_offset isn't applied when binding the
>> callee's 'this', so the body is entered with the unadjusted (offset - 8)
>> receiver. Pointer identity is then lost interprocedurally (i.e a 'delete
>> this' in the body doesn't map back to the caller's region thus return
>> values aren't observed).
>> This means devirt-multiple-inheritance-1.C only asserts the throw/leak
>> side-effect rather than a return-value __analyzer_eval (the eval comes
>> back UNKNOWN).
>>
>> Applying the thunk fixed_offset before binding 'this' would fix the
>> interprocedural case, but that's out of scope here and left as
>> follow-up.
>
> As I understand the above, the approach in your patch aims for
> simplicity at the cost of some precision for the multiple inheritance
> case. These seems like the correct trade-off to me.
>
Yes.
>>
>>
>> Regardless, a possible approach might be something like this:
>>
>> thunk_info::get(node) exposes fixed_offset (plus virtual_offset for _ZTv
>> virtual-base thunks). When a call resolves through a thunk, the receiver
>> region needs adjusting by -fixed_offset before it's bound to the
>> callee's 'this', so the body sees the offset-0 C* rather than the
>> offset-8 B* subobject.
>> The offset has to be threaded from get_fndecl_for_virtual_call to the
>> frame-binding point (push_frame / arg binding), since that's where
>> 'this' is set up. Although virtual-base thunks need the vtable-loaded
>> offset, so having fixed_offset alone probably won't work either.
>>
>>
>> I'm not sure how this should be done in practice, it seems to fight a
>> bit against the way things are modeled right now.
>> It might also be a good idea make a new BZ associated to multiple
>> inheritance support for this, and add the above there for future work.
>
> Sounds like the correct approach.
>
I'll open a BZ entry for this then.
>> +/* Attempt to get the fndecl from an OBJ_TYPE_REF instance, if known, or
>> + NULL_TREE otherwise.
>> +
>> + This method relies on being able to access the value stored in the vptr
>> + field of the associated OBJ_TYPE_REF_OBJECT, and deriving the original
>> + fndecl from that.
>> +
>> + The devirtualization logic is similar to vtable_pointer_value_to_vtable.
>> */
>
> I think the comment could be expanded it a bit here. If I'm reading
> things right, the code is looking at the value of "obj->vfield", and
> seeing if it's of the form "decl_ptr + constant", and if so, using
> gimple_get_virt_method_for_vtable on that to get at a fndecl. (Am I
> reading it right?)
>
Yes. I can expand the comment; initially I actually reduced it to this,
but I guess it would've been better to write a more lengthy description.
>> diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
>> index fd947da65e8..d1e2b014d0e 100644
>> --- a/gcc/analyzer/region-model.h
>> +++ b/gcc/analyzer/region-model.h
>> @@ -509,6 +509,8 @@ class region_model
>>
>> tree get_fndecl_for_call (const gcall &call,
>> region_model_context *ctxt);
>> + tree get_fndecl_for_virtual_call (const_tree fn_ptr,
>> + region_model_context *ctxt);
>>
>> void get_regions_for_current_frame (auto_vec<const decl_region *> *out)
>> const;
>> static void append_regions_cb (const region *base_reg,
>> diff --git a/gcc/testsuite/g++.dg/analyzer/devirt-1.C
>> b/gcc/testsuite/g++.dg/analyzer/devirt-1.C
>> new file mode 100644
>> index 00000000000..cf8a8a40fb0
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/analyzer/devirt-1.C
>> @@ -0,0 +1,24 @@
>> +#include "../../gcc.dg/analyzer/analyzer-decls.h"
>> +struct Base
>> +{
>> + virtual int base_impl () { return 0; }
>> + virtual int derived_impl () { return 0; }
>> +};
>> +struct Derived : public Base
>> +{
>> + int base_impl () { return 1; }
>> + int derived_impl () { return 60; }
>> +};
>> +Base *
>> +make_derived ()
>> +{
>> + return new Derived ();
>> +}
>> +void
>> +test ()
>> +{
>> + Base *f = make_derived ();
>> + __analyzer_eval (f->base_impl () == 1); // { dg-warning "TRUE" }
>> + __analyzer_eval (f->derived_impl () == 60); // { dg-warning "TRUE" }
>> + delete f;
>> +}
>
> What's up with the naming of base_impl and derived_impl? These are
> both overridden by Derived; was the intent to have one that isn't
> overridden and one that is?
>
Thanks for pointing this out; I actually had remade this test with
different names and it was meant to work differently, but I must've
reverted those changes while editing the patch description and so on,
and ended up with this nonsense test. I'll resubmit with a new test.
> With all these various test cases, we need to be sure that the analyzer
> is doing the devirt, that the optimizer isn't doing the devirt for us.
> I think we can guarantee this my marking make_derived with
> __attribute__((noipa)) so that the optimizer can't inline it into test
> and so test can't know what subclass *f is.
>
I'll try doing this.
> I'd prefer more test coverage (perhaps split "test" into "test_base"
> and "test_derived" with a "make_base" similar to "make_derived") but
> that can be added as followups.
>
Will do.
> Other than the above nitpicks, the patch looks good to me; thanks
> again.
>
> Dave
>
Thanks for the feedback. I'll resubmit the patch whenever I can (might
be a bit busy next week but the changes aren't big so it should be
quick).
Egas
