Re: [PATCH 1/2] [gdb/symtab] Fix style issues in v9 .gdb_index section support

[Tom de Vries <tdevries@suse.de>]

On 10/19/23 18:55, Thiago Jung Bauermann wrote:
> 
> Hello Tom,
> 
> Tom de Vries <tdevries@suse.de> writes:
> 
>> @@ -1217,7 +1217,7 @@ write_shortcuts_table (cooked_index *table, data_buf& shortcuts,
>>   	}
>>       }
>>   
>> -  shortcuts.append_uint (4, BFD_ENDIAN_LITTLE, dw_lang);
>> +  shortcuts.append_offset (dw_lang);
> 
> This is a functional change. Is it intentional? If so, it'd be better as
> a separate patch.
> 

It is not supposed to be functional change given that append_offset is 
defined as:
...
   void append_offset (offset_type value)
   {
     append_uint (sizeof (value), BFD_ENDIAN_LITTLE, value);
   }
...
and offset_type as:
...
typedef uint32_t offset_type;
...

>>     shortcuts.append_offset (main_name_offset);
>>   }
>>   
>> diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c
>> index b96eaa96e23..34c5dfd5803 100644
>> --- a/gdb/dwarf2/read-gdb-index.c
>> +++ b/gdb/dwarf2/read-gdb-index.c
>> @@ -777,10 +777,10 @@ static void
>>   set_main_name_from_gdb_index (dwarf2_per_objfile *per_objfile,
>>   			      mapped_gdb_index *index)
>>   {
>> -  const auto expected_size = 4 + sizeof (offset_type);
>> +  const auto expected_size = 2 * sizeof (offset_type);
> 
> This is also a functional change.
> 

Given that we have sizeof (offset_type) == 4, we rewrite "4 + 4" into "2 
* 4".

>>     if (index->shortcut_table.size () < expected_size)
>>       /* The data in the section is not present, is corrupted or is in a version
>> -     * we don't know about. Regardless, we can't make use of it. */
>> +       we don't know about. Regardless, we can't make use of it. */
> 
> One further style issue that could be fixed here is the lack of double
> space after period.
> 
>>       return;
>>   
>>     auto ptr = index->shortcut_table.data ();
>> @@ -794,7 +794,7 @@ set_main_name_from_gdb_index (dwarf2_per_objfile *per_objfile,
>>     if (dw_lang == 0)
>>       {
>>         /* Don't bother if the language for the main symbol was not known or if
>> -       * there was no main symbol at all when the index was built. */
>> +	 there was no main symbol at all when the index was built. */
>>         return;
> 
> Also here, double space after period is lacking.
> 
>>       }
>>     ptr += 4;
> 
>> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
>> index 1d9432c5c11..c92474d8b9d 100644
>> --- a/gdb/dwarf2/read.h
>> +++ b/gdb/dwarf2/read.h
>> @@ -246,11 +246,11 @@ struct dwarf2_per_cu_data
>>     std::vector <dwarf2_per_cu_data *> *imported_symtabs = nullptr;
>>   
>>     /* The original DW_LANG_* value of the CU, as provided to us by
>> -   * DW_AT_language. It is interesting to keep this value around in cases where
>> -   * we can't use the values from the language enum, as the mapping to them is
>> -   * lossy, and, while that is usually fine, things like the index have an
>> -   * understandable bias towards not exposing internal GDB structures to the
>> -   * outside world, and so prefer to use DWARF constants in their stead. */
>> +     DW_AT_language. It is interesting to keep this value around in cases where
>> +     we can't use the values from the language enum, as the mapping to them is
>> +     lossy, and, while that is usually fine, things like the index have an
>> +     understandable bias towards not exposing internal GDB structures to the
>> +     outside world, and so prefer to use DWARF constants in their stead. */
> 
> I think the lack of double space after period here are fixed in the
> other patch you sent.
> 

Yes, those are fixed in that patch, I'll commit soon.

Thanks for the review.
- Tom