On Fri, 2026-07-10 at 12:00 +0100, Egas Ribeiro wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu.
Hi Egas. Thanks for the patch. Various comments inline below... > This patch adds support for devirtualization when the concrete subtype > is trackable, but it doesn't try to track possible subtypes, thus won't > try to check if each possible sub method could throw/leak/etc when the > most derived type isn't known. > Supporting that would add more overhead than is probably desirable for > the analyzer, but either way it should be considered on a different > patch. > > Some of the work done in get_fndecl_for_virtual_call overlaps with what > we will use for dynamic_cast support, and a patch for that might want to > separate some of the get_fndecl_for_virtual_call logic into a helper. > > > 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. > > > 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. > > -- >8 -- > > The analyzer doesn't currently support virtual function call resolution, > and has no way to devirtualize concrete calls. > > This patch adds support for devirtualization of virtual methods by > adding logic around OBJ_TYPE_REF calls and using the information it > tracks to link the calls back to the concrete subclass implementation > stored in the OBJ_TYPE_REF_OBJECT's vptr field. > > This approach relies on the pre-existing store binding mechanism to keep > track of which _ZTV* instance the object's vptr field points to, and > thus doesn't require any additional modeling of the dynamic type of the > object to resolve calls. > > can_throw_p is also extended to consider thunks that wrap virtual > methods when dealing with multiple inheritance, and now checks > TREE_NOTHROW for more accurate results (i.e considers noexcept > attributes on methods). > > PR analyzer/97114 > > gcc/analyzer/ChangeLog: > > * region-model.cc (can_throw_p): improve NOTHROW detection and > consider the thunk case. > (region_model::get_fndecl_for_virtual_call): New function. > (region_model::get_fndecl_for_call): Update to use > get_fndecl_for_virtual_call on OBJ_TYPE_REF. > * region-model.h (class region_model): New decl. > > gcc/testsuite/ChangeLog: > > * g++.dg/analyzer/devirt-1.C: New test. > * g++.dg/analyzer/devirt-multiple-inheritance-1.C: New test. > * g++.dg/analyzer/devirt-noexcept-1.C: New test. > * g++.dg/analyzer/devirt-unknown-1.C: New test. > > Signed-off-by: Egas Ribeiro <[email protected]> > --- > gcc/analyzer/region-model.cc | 82 +++++++++++++++++++ > gcc/analyzer/region-model.h | 2 + > gcc/testsuite/g++.dg/analyzer/devirt-1.C | 24 ++++++ > .../analyzer/devirt-multiple-inheritance-1.C | 31 +++++++ > .../g++.dg/analyzer/devirt-noexcept-1.C | 26 ++++++ > .../g++.dg/analyzer/devirt-unknown-1.C | 16 ++++ > 6 files changed, 181 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/analyzer/devirt-1.C > create mode 100644 > gcc/testsuite/g++.dg/analyzer/devirt-multiple-inheritance-1.C > create mode 100644 gcc/testsuite/g++.dg/analyzer/devirt-noexcept-1.C > create mode 100644 gcc/testsuite/g++.dg/analyzer/devirt-unknown-1.C > > diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc > index d88651fc33b..ffa4128027c 100644 > --- a/gcc/analyzer/region-model.cc > +++ b/gcc/analyzer/region-model.cc > @@ -48,6 +48,9 @@ along with GCC; see the file COPYING3. If not see > #include "channels.h" > #include "value-relation.h" > #include "range-op.h" > +#include "ipa-utils.h" > +#include "gimple-iterator.h" > +#include "gimple-fold.h" > > #include "text-art/tree-widget.h" > > @@ -2210,6 +2213,14 @@ can_throw_p (const gcall &call, tree fndecl) > > if (fndecl) > { > + /* If we are checking a thunk, we want to verify whether or not the > + underlying function is nothrow. */ > + if (cgraph_node *n = cgraph_node::get (fndecl)) > + fndecl = n->function_symbol ()->decl; > + > + if (TREE_NOTHROW (fndecl)) > + return false; > + > const function_set fs = get_fns_assumed_not_to_throw (); > if (fs.contains_decl_p (fndecl)) > return false; > @@ -6479,6 +6490,71 @@ region_model::can_merge_with_p (const region_model > &other_model, > return true; > } > > +/* 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?) > + > +tree > +region_model::get_fndecl_for_virtual_call (const_tree obj_type_ref, > + region_model_context *ctxt) > +{ > + tree obj = OBJ_TYPE_REF_OBJECT (obj_type_ref); > + tree obj_type = obj_type_ref_class (obj_type_ref); > + if (!obj_type) > + return NULL_TREE; > + tree vfield = TYPE_VFIELD (obj_type); > + if (!vfield) > + return NULL_TREE; > + > + const svalue *obj_sval = get_rvalue (obj, ctxt); > + const region *obj_reg = deref_rvalue (obj_sval, obj, ctxt); > + const region *vptr_reg = m_mgr->get_field_region (obj_reg, vfield); > + > + const svalue *vptr_sval = get_store_value (vptr_reg, ctxt); > + while (const svalue *cast = vptr_sval->maybe_undo_cast ()) > + vptr_sval = cast; > + > + const binop_svalue *b = vptr_sval->dyn_cast_binop_svalue (); > + if (!b || b->get_op () != POINTER_PLUS_EXPR) > + return NULL_TREE; > + > + vptr_sval = b->get_arg0 (); > + const svalue *offset_sval = b->get_arg1 (); > + > + tree offset_const = offset_sval->maybe_get_constant (); > + if (!offset_const || TREE_CODE (offset_const) != INTEGER_CST) > + return NULL_TREE; > + unsigned HOST_WIDE_INT offset = tree_to_uhwi (offset_const); > + > + const region_svalue *vptr = vptr_sval->dyn_cast_region_svalue (); > + /* Give up if we have a conjured vptr. */ > + if (!vptr) > + return NULL_TREE; > + > + tree vtable = vptr->get_pointee ()->maybe_get_decl (); > + if (!vtable) > + return NULL_TREE; > + > + unsigned HOST_WIDE_INT token > + = tree_to_uhwi (OBJ_TYPE_REF_TOKEN (obj_type_ref)); > + bool can_refer; > + tree vptr_fn > + = gimple_get_virt_method_for_vtable (token, vtable, offset, &can_refer); > + if (!vptr_fn || !can_refer) > + return NULL_TREE; > + > + if (cgraph_node *node = cgraph_node::get_create (vptr_fn)) > + if (const cgraph_node *ultimate = node->ultimate_alias_target ()) > + return ultimate->decl; > + > + return NULL_TREE; > +} > + > /* Attempt to get the fndecl used at CALL, if known, or NULL_TREE > otherwise. */ > > @@ -6489,6 +6565,12 @@ region_model::get_fndecl_for_call (const gcall &call, > tree fn_ptr = gimple_call_fn (&call); > if (fn_ptr == NULL_TREE) > return NULL_TREE; > + > + /* Handle OBJ_TYPE_REF so that we can try to find the definition for a > virtual > + call and treat it like any other call. */ > + if (TREE_CODE (fn_ptr) == OBJ_TYPE_REF) > + return get_fndecl_for_virtual_call (fn_ptr, ctxt); > + > const svalue *fn_ptr_sval = get_rvalue (fn_ptr, ctxt); > if (const region_svalue *fn_ptr_ptr > = fn_ptr_sval->dyn_cast_region_svalue ()) > 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? 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'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. Other than the above nitpicks, the patch looks good to me; thanks again. Dave > diff --git a/gcc/testsuite/g++.dg/analyzer/devirt-multiple-inheritance-1.C > b/gcc/testsuite/g++.dg/analyzer/devirt-multiple-inheritance-1.C > new file mode 100644 > index 00000000000..df7e82855a2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/devirt-multiple-inheritance-1.C > @@ -0,0 +1,31 @@ > +#include "../../gcc.dg/analyzer/analyzer-decls.h" > + > +struct A > +{ > + virtual void f () {} > +}; > +struct B > +{ > + virtual void g () { throw 1; } > +}; > + > +struct C : public A, public B > +{ > + void f () {} > + void g () {} > +}; > + > +C * > +make_c () > +{ > + return new C (); > +} > + > +void > +test () > +{ > + C *c = make_c (); > + B *b = c; > + b->g (); // { dg-bogus "leak" } > + delete c; > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/devirt-noexcept-1.C > b/gcc/testsuite/g++.dg/analyzer/devirt-noexcept-1.C > new file mode 100644 > index 00000000000..82ee0378e39 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/devirt-noexcept-1.C > @@ -0,0 +1,26 @@ > +#include "../../gcc.dg/analyzer/analyzer-decls.h" > +// { dg-additional-options "-std=c++11" } > + > +struct Base > +{ > + virtual void __analyzer_foo () = 0; > +}; > + > +struct Derived : public Base > +{ > + void __analyzer_foo () noexcept override {} > +}; > + > +Base * > +make_derived () > +{ > + return new Derived (); > +} > + > +void > +test () > +{ > + Base *f = make_derived (); > + f->__analyzer_foo (); // { dg-bogus "leak" } > + delete f; > +} > diff --git a/gcc/testsuite/g++.dg/analyzer/devirt-unknown-1.C > b/gcc/testsuite/g++.dg/analyzer/devirt-unknown-1.C > new file mode 100644 > index 00000000000..991159e57d7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/analyzer/devirt-unknown-1.C > @@ -0,0 +1,16 @@ > +#include "../../gcc.dg/analyzer/analyzer-decls.h" > + > +struct Base > +{ > + virtual void __analyzer_foo () { throw 1; } > +}; > + > +extern Base *get_base (void); // unknown dynamic type > + > +void > +test () > +{ > + Base *f = get_base (); > + // vptr unknown (won't devirt) > + f->__analyzer_foo (); // { dg-bogus "leak" } > +}
