Re: [RFC 02/13] i387-tdep: Add function to read XSAVE layout from NT_X86_CPUID

[John Baldwin <jhb@FreeBSD.org>]

On 10/16/23 2:17 AM, Lancelot SIX wrote:
> Hi,
> 
> I am not familiar with XSAVE details, but I have pure c++ style comments
> below.

Thanks, I've generally accepted the changes aside from a few modifications below.
  
> On Mon, Oct 09, 2023 at 11:36:04AM -0700, John Baldwin wrote:
>> This can be used by x86 arches to determine the XSAVE layout instead
>> of guessing based on the XCR0 mask and XSAVE register note size.
>> ---
>>   gdb/i387-tdep.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   gdb/i387-tdep.h |   8 +++
>>   2 files changed, 140 insertions(+)
>>
>> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
>> index 47667da21c7..1eac2b6bd2a 100644
>> --- a/gdb/i387-tdep.c
>> +++ b/gdb/i387-tdep.c
>> +  size_t size = bfd_section_size (section);
>> +  if (size == 0 || (size % (6 * 4)) != 0)
>> +    return {};
>> +
>> +  char contents[size];
> 
> If I remember correctly, VLAs are not a C++ feature (but are supported
> as a GCC extension
> https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html).  I am unsure
> if GDB has a policy regarding the use of extensions, so maybe this is
> fine.  Otherwise, you could use a std::vector instead (it comes with a
> dynamic allocation, but I am not too concerned at this is hardly on a
> performance critical path)
> 
>     std::vector<char> contents (size);

I've used a gdb::byte_vector instead of a plain std::vector<>.

>> +      if (map.count (cpuid_key (leaf, count)) != 0)
>> +	{
>> +	  warning (_("Duplicate cpuid leaf %#x,%#x"), leaf, count);
>> +	  return {};
>> +	}
>> +      map.emplace (cpuid_key (leaf, count),
>> +		   cpuid_values (eax, ebx, ecx, edx));
> 
> As Simon pointed out, there are two lookups here, where you can get away
> with just one.  However, this is C++17 only which is not [yet] available
> in GDB.  Instead, you can use the value returned by emplace to know if
> an insertation has been done or not:
> 
>    auto emplace_result = map.emplace (cpuid_key (leaf, count),
>                                       cpuid_values (eax, ebx, ecx, edx));
>    if (!emplace_result.second)
>      {
>        warning (_("Duplicate cpuid leaf %#x,%#x"), leaf, count);
>        return {};
>      }

I'm going to assume that C++17 will land first in GDB before this and just
go with try_emplace for now.  If I end up backporting this to GDB 14 (which
I don't currently anticipate), then this wil be nicer for that.

-- 
John Baldwin