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

[John Baldwin <jhb@FreeBSD.org>]

On 10/11/23 9:27 PM, Simon Marchi wrote:
> 
> 
> On 2023-10-09 14:36, 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.
> 
> Just some nits below:
> 
>> +typedef std::unordered_map<cpuid_key, cpuid_values> cpuid_map;
> 
> For new stuff I would suggest:
> 
>    using cpuid_map = std::unordered_map<cpuid_key, cpuid_values>;

Ok.

>> +
>> +static cpuid_map
>> +i387_parse_cpuid_from_core (bfd *bfd)
>> +{
>> +  asection *section = bfd_get_section_by_name (bfd, ".reg-x86-cpuid");
>> +  if (section == nullptr)
>> +    return {};
>> +
>> +  size_t size = bfd_section_size (section);
>> +  if (size == 0 || (size % (6 * 4)) != 0)
> 
> That 4 could be `sizeof (uint32_t)`.  And `6 * 4` appears below again,
> it could a constexpr value with a name like entry_size.

Ok.

>> +    return {};
>> +
>> +  char contents[size];
>> +  if (!bfd_get_section_contents (bfd, section, contents, 0, size))
>> +    {
>> +      warning (_("Couldn't read `.reg-x86-cpuid' section in core file."));
>> +      return {};
>> +    }
>> +
>> +  cpuid_map map;
>> +  size_t index = 0;
>> +  while (index < size)
>> +    {
>> +      uint32_t leaf = bfd_get_32 (bfd, contents + index);
>> +      uint32_t count = bfd_get_32 (bfd, contents + index + 4);
>> +      uint32_t eax = bfd_get_32 (bfd, contents + index + 8);
>> +      uint32_t ebx = bfd_get_32 (bfd, contents + index + 12);
>> +      uint32_t ecx = bfd_get_32 (bfd, contents + index + 16);
>> +      uint32_t edx = bfd_get_32 (bfd, contents + index + 20);
>> +
>> +      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));
> 
> This could be slightly more optimal using map::try_emplace (to avoid
> having two map lookups).

Ok.

Presumably C++17 will be required before this series lands, and I
don't plan to backport it to GDB 14.  (If I did it could also turn
back into a loop + emplace though as part of the backport.)

-- 
John Baldwin