Re: [PATCH] Add infcall support for C++ constructor-style expressions

[Keith Seitz <keiths@redhat.com>]

Hi,

On 4/21/26 6:41 AM, Andrew Burgess wrote:
> Keith Seitz <keiths@redhat.com> writes:
>
 >> diff --git a/gdb/c-exp.y b/gdb/c-exp.y>> index 
a4a910df712..e17ee2d3c12 100644
>> --- a/gdb/c-exp.y
>> +++ b/gdb/c-exp.y
>> @@ -3114,6 +3140,34 @@ static int popping;
>>      built up.  */
>>   static auto_obstack name_obstack;
>>   
>> +/* Return TYPENAME_CTOR only when the next token is '(', so this
>> +   token is used solely for constructor calls.  Otherwise return TYPENAME.
>> +   NAME_END is the character just past the name (e.g. yylval.sval.ptr +
>> +   yylval.sval.length).  */
>> +
>> +static int
>> +typename_token_for (struct parser_state *par_state, struct type *type,
>> +		    const char *name_end)
>> +{
>> +  if (type == nullptr
>> +      || par_state->language ()->la_language != language_cplus)
>> +    return TYPENAME;
>> +  type = check_typedef (type);
>> +  if (type->code () != TYPE_CODE_STRUCT && type->code () != TYPE_CODE_UNION)
>> +    return TYPENAME;
>> +  /* Only return TYPENAME_CTOR when followed by '('.  */
>> +  if (name_end == nullptr)
>> +    return TYPENAME;
>> +  {
>> +    const char *p = name_end;
>> +    while (*p == ' ' || *p == '\t' || *p == '\n' || *p == '\r')
> 
> Would 'c_isspace (*p)' work here?  It's what we use in similar cases
> within this file.

Absolutely!

>> diff --git a/gdb/eval.c b/gdb/eval.c
>> index 7beff554ed4..e988b954059 100644
>> --- a/gdb/eval.c
>> +++ b/gdb/eval.c
>> @@ -1869,6 +1869,61 @@ type_operation::evaluate (struct type *expect_type, struct expression *exp,
>>       error (_("Attempt to use a type name as an expression"));
>>   }
>>   
>> +value *
>> +type_operation::evaluate_funcall (struct type *expect_type,
>> +				  struct expression *exp,
>> +				  enum noside noside,
>> +				  const std::vector<operation_up> &args)
>> +{
>> +  struct type *type = std::get<0> (m_storage);
>> +  type = check_typedef (type);
>> +
>> +  /* Constructor-style call Type(args) is only for C++ aggregate types.  */
>> +  gdb_assert (exp->language_defn->la_language == language_cplus);
>> +
>> +  const char *name = type->name ();
>> +  if (name == nullptr)
>> +    error (_("Cannot call constructor of unnamed type"));
> 
> Thinking about this error was interesting.
[snip]
> Then I extended the type_exp rule like:
> 
>    type_exp:
>    	... snip ...
>    	|	TYPENAME_CTOR
>    			{
>    			  pstate->push_new<type_operation> ($1.type);
>    			}
> 
> All of the new tests still pass...
> 
> ... but the decltype example still doesn't work.  But by this time I was
> curious, clearly my actual example with global_var is never going to
> work, the global_var type doesn't have a constructor, but what if
> global_var was a type that did have a constructor?  The decltype trick
> should be workable, right?
> 
> So, I guess the question I'm circling around here is, instead of adding
> typename_for_ctor, did you try just using the existing type_exp rule?
> Even if we didn't get the decltype trick working in the original commit,
> it feels like going through type_exp would leave that as a possibility
> for the future, but going with typename_for_ctor feels like it will make
> that harder.

I did at one time use a similar approach. I think what happened is that
I ran into issues and started breaking down/isolating my changes to make
sure that it wasn't some other production messing me up. Alas, I did
not actually go back and try to integrate my working solution. I will
investigate further for v2.

> 
>> +
>> +  /* Get the constructor name from the type name.  */
>> +  gdb::unique_xmalloc_ptr<char> ctor_name_ptr = cp_func_name (name);
>> +  const char *ctor_name = (ctor_name_ptr != nullptr) ? ctor_name_ptr.get () : name;
> 
> This line seems a little long, maybe wrap at the '=' ?
> 
>> +
>> +  if (!overload_resolution)
>> +    return operation::evaluate_funcall (expect_type, exp, noside, args);
> 
> I've pretty sure that this path, calling operation::evaluate_funcall,
> will always result in an error like:
> 
>    (gdb) set overload-resolution off
>    (gdb) p U(34)
>    Attempt to use a type name as an expression
>    (gdb)
> 
> I wonder if it would be clearer to the user to just do:
> 
>    if (!overload_resolution)
>      error (_("Constructor calls require 'set overload-resolution on'"));

That is a much more user friendly approach that I will adopt in v2.

> I didn't see any tests with 'set overload-resolution off' in use, so it
> wasn't clear if there's maybe some path where the function call as you
> have it can do anything other than throw an error?

I'll add this test, too.

>> +  std::vector<value *> argvec (1 + args.size ());
>> +  value *this_ptr;
>> +  if (noside == EVAL_AVOID_SIDE_EFFECTS)
>> +    this_ptr = value::zero (lookup_pointer_type (type), lval_memory);
>> +  else
>> +    {
>> +      value *alloc_val = value_allocate_space_in_inferior (type->length ());
>> +      this_ptr = value_from_pointer (lookup_pointer_type (type),
>> +				     value_as_long (alloc_val));
>> +    }
>> +  argvec[0] = this_ptr;
>> +  for (size_t i = 0; i < args.size (); ++i)
>> +    argvec[i + 1] = args[i]->evaluate_with_coercion (exp, noside);
>> +  gdb::array_view<value *> arg_view = argvec;
>> +
>> +  value *callee = nullptr;
>> +  int static_memfuncp;
>> +  find_overload_match (arg_view, ctor_name, METHOD,
>> +		       &argvec[0], nullptr, &callee, nullptr,
>> +		       &static_memfuncp, 0, noside);
> 
> It would not be usual for constructors to be static, but we should
> probably handle the case where static_memfuncp is true, even if it is
> just:
> 
>    if (static_memfuncp)
>      error (_("Constructor %s is unexpectedly marked static"), name);
> 
> Bonus points for using the DWARF assembler to exercise this error case,
> but I don't think that's a hard requirement, I see this error more as a
> glorified todo marker -- if we ever find a case where this is triggers
> then we can understand it, and fix GDB to match.

I will add this.

> 
>> +  if (callee == nullptr)
>> +    error (_("Cannot resolve constructor %s to any overloaded instance"),
>> +	   name);
> 
> I don't think this error case is being tested.  Can you add a test case
> for this?

Boy, I've spent a lot of my day today trying to trigger this error. I am
convinced it cannot be done. I'm passing METHOD to find_overload_match, 
which will always call error() when no matching symbol is found. Thus I
think it best to replace this error check with an assertion.

> 
>> +
>> +  if (noside == EVAL_AVOID_SIDE_EFFECTS)
>> +    return value::zero (type, not_lval);
>> +
>> +  evaluate_subexp_do_call (exp, noside, callee, arg_view,
>> +			   nullptr, expect_type);
>> +  return value_ind (this_ptr);
>> +}
>> +
>>   }
>>   
>>   /* A helper function for BINOP_ASSIGN_MODIFY.  */
>> diff --git a/gdb/testsuite/gdb.cp/infcall-ctors.cc b/gdb/testsuite/gdb.cp/infcall-ctors.cc
>> new file mode 100644
>> index 00000000000..053542ec5ea
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/infcall-ctors.cc
>> @@ -0,0 +1,128 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright (C) 2026 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +struct S {
> 
> The '{' should be on a new line.  This mistake is repeated throughout
> this test, I'll not point them all out.
> 
>> +  int x;
>> +  explicit S (int n = 0) : x (n) {}
>> +  S operator+ (int n) const { return S (x + n); }
>> +};
>> +
>> +typedef S S_td;
>> +using S_u = S;
>> +
>> +static int add (const struct S &s1, const struct S &s2) {
> 
> Newline before add, opening '{' on its own line.  Unless this layout is
> needed for the test?  In which case a comment is needed.

Nope -- just like a lot of the other silly formatting issues, I simply
over-relied on my new editor to "do the right thing." I'll fix all
the errors you've mentioned.

Thank you for your review! [I'll submit a v2.]

Keith