[RFC PATCH v4 09/15] GDB tdesc: Add vector type with number of elements given by math expression.

[Gerlicher, Klaus]

Hi Thiago,

> Hello Klaus,
> 
> "Gerlicher, Klaus" <klaus.gerlicher@intel.com> writes:
> 
> > Just pointing out one thing I noticed when studying this patch, inline at
> > evaluate_locexpr.
> 
> Thanks for the observation.
> 
> >> -----Original Message-----
> >> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> >> Sent: Saturday, November 2, 2024 3:56 AM
> >> To: gdb-patches@sourceware.org
> >> Subject: [RFC PATCH v4 09/15] GDB tdesc: Add vector type with number of
> >> elements given by math expression.
> >>
> >> +long
> >> +evaluate_locexpr (const gdb::array_view<const gdb_byte> locexpr,
> >> +		  const struct regcache *regcache)
> >> +{
> >> +  std::vector<long> stack;
> >> +
> >> +  for (auto it = locexpr.begin (); it != locexpr.end ();)
> >> +    {
> >> +      gdb_byte op = *it;
> >> +      long result;
> >> +
> >> +      it++;
> >> +      switch (op)
> >> +	{
> >> +	case DW_OP_div:
> >> +	  {
> >> +	    long divisor = stack.back ();
> >> +	    stack.pop_back ();
> >> +	    long dividend = stack.back ();
> >> +	    stack.pop_back ();
> >> +	    result = dividend / divisor;
> >> +	    break;
> >> +	  }
> >> +	case DW_OP_mul:
> >> +	  {
> >> +	    long multiplicand = stack.back ();
> >> +	    stack.pop_back ();
> >> +	    long multiplier = stack.back ();
> >> +	    stack.pop_back ();
> >> +	    result = multiplier * multiplicand;
> >> +	    break;
> >> +	  }
> >> +	case DW_OP_lit0:
> >> +	case DW_OP_lit2:
> >> +	case DW_OP_lit4:
> >> +	case DW_OP_lit8:
> >> +	  result = op - DW_OP_lit0;
> >> +	  break;
> >> +	case DW_OP_bregx:
> >> +	  {
> >> +	    uint64_t reg;
> >> +	    int read = read_uleb128_to_uint64 (it, locexpr.end (), &reg);
> >> +	    gdb_assert (read != 0);
> >> +
> >> +	    it += read;
> >> +
> >> +	    int64_t offset;
> >> +	    read = read_sleb128_to_int64 (it, locexpr.end (), &offset);
> >> +	    gdb_assert (read != 0);
> >> +
> >> +	    it += read;
> >> +
> >> +	    const char *arch = tdesc_architecture_name (regcache->tdesc);
> >> +	    reg = tdesc_dwarf_reg_to_regnum (arch, reg);
> >> +	    gdb_assert (reg != -1);
> >> +
> >> +	    register_status reg_status = regcache->get_register_status (reg);
> >> +	    gdb_assert (reg_status == REG_VALID);
> >
> > evaluate_locexpr (const gdb::array_view<const gdb_byte> locexpr, const
> struct regcache
> > *regcache) pushed longs on the stack but the
> > register used for vector length could be any size. Vg in your ARM SVE case
> seems to be
> > 64bit incidentally but other archs may have different sizes here.
> 
> Indeed I am assuming that any register involved in a register size
> expression is at most 64-bits wide. Do AMX or LargeGRF reference bigger
> registers in their size expressions? Can we at least assume that the
> contents of said registers will be less than 64 bits wide? :)
> 

It would make sense to be very generic here and allow any type of register. I think you just left this code as simple as possible for the RFC. 

In the end the method evaluating the expression (see also suggested alternative in other reply https://sourceware.org/pipermail/gdb-patches/2025-February/215521.html) would potentially drop this and so I see no need to worry anymore about it.

Thanks
Klaus

> >> +	    regcache->raw_collect (reg,
> >> +				   gdb::make_array_view ((gdb_byte *)
> >> &result,
> >> +							 sizeof (result)));
> >> +	    result += offset;
> >> +	    break;
> >> +	  }
> >> +	default:
> >> +	  gdb_assert_not_reached ("Unsupported locexpr operation: %c\n",
> >> op);
> >> +	}
> >> +
> >> +      stack.push_back (result);
> >> +    }
> >> +
> >> +  return stack.back ();
> >> +}
> 
> --
> Thiago
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928