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

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

[Tom de Vries]

Post-commit review pointed out a few style issues in commit 8b9c08eddac
("[gdb/symtab] Add name_of_main and language_of_main to the DWARF index").

Fix these.

Tested on x86_64-linux.

Reported-By: Tom Tromey <tom@tromey.com>
---
 gdb/dwarf2/index-write.c    |  8 ++++----
 gdb/dwarf2/read-gdb-index.c |  8 ++++----
 gdb/dwarf2/read.c           |  4 ++--
 gdb/dwarf2/read.h           | 10 +++++-----
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 6ea4217fb22..6a21af80c3f 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1196,12 +1196,12 @@ write_cooked_index (cooked_index *table,
 /* Write shortcut information. */
 
 static void
-write_shortcuts_table (cooked_index *table, data_buf& shortcuts,
-		       data_buf& cpool)
+write_shortcuts_table (cooked_index *table, data_buf &shortcuts,
+		       data_buf &cpool)
 {
   const auto main_info = table->get_main ();
   size_t main_name_offset = 0;
-  dwarf_source_language dw_lang = (dwarf_source_language)0;
+  dwarf_source_language dw_lang = (dwarf_source_language) 0;
 
   if (main_info != nullptr)
     {
@@ -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);
   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);
   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. */
     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;
     }
   ptr += 4;
@@ -803,7 +803,7 @@ set_main_name_from_gdb_index (dwarf2_per_objfile *per_objfile,
   const auto name_offset = extract_unsigned_integer (ptr,
 						     sizeof (offset_type),
 						     BFD_ENDIAN_LITTLE);
-  const auto name = (const char*) (index->constant_pool.data () + name_offset);
+  const auto name = (const char *) (index->constant_pool.data () + name_offset);
 
   set_objfile_main_name (per_objfile->objfile, name, (enum language) lang);
 }
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d4aec19d31d..085ebad3402 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -21727,7 +21727,7 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   /* Set the language we're debugging.  */
   attr = dwarf2_attr (comp_unit_die, DW_AT_language, cu);
   enum language lang;
-  dwarf_source_language dw_lang = (dwarf_source_language)0;
+  dwarf_source_language dw_lang = (dwarf_source_language) 0;
   if (cu->producer != nullptr
       && strstr (cu->producer, "IBM XL C for OpenCL") != NULL)
     {
@@ -21748,7 +21748,7 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   else if (attr != nullptr)
     {
       lang = dwarf_lang_to_enum_language (attr->constant_value (0));
-      dw_lang = (dwarf_source_language)attr->constant_value (0);
+      dw_lang = (dwarf_source_language) attr->constant_value (0);
     }
   else
     lang = pretend_language;
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. */
   dwarf_source_language dw_lang;
 
   /* Return true of IMPORTED_SYMTABS is empty or not yet allocated.  */

base-commit: f6ca448ab70c52e923b7010aecdf7be9c0d4d4fc
-- 
2.35.3

[PATCH 2/2] [gdb/symtab] Fix two style issues in gdb/dwarf2/index-write.c

[Tom de Vries]

While reviewing gdb/dwarf2/index-write.c I noticed two style issues.

Fix these.

Tested on x86_64-linux.
---
 gdb/dwarf2/index-write.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index 6a21af80c3f..bac4a6c6934 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -1300,8 +1300,8 @@ write_gdbindex (dwarf2_per_bfd *per_bfd, cooked_index *table,
   data_buf shortcuts;
   write_shortcuts_table (table, shortcuts, constant_pool);
 
-  write_gdbindex_1(out_file, objfile_cu_list, types_cu_list, addr_vec,
-		   symtab_vec, constant_pool, shortcuts);
+  write_gdbindex_1 (out_file, objfile_cu_list, types_cu_list, addr_vec,
+		    symtab_vec, constant_pool, shortcuts);
 
   if (dwz_out_file != NULL)
     write_gdbindex_1 (dwz_out_file, dwz_cu_list, {}, {}, {}, {}, {});
@@ -1575,7 +1575,7 @@ save_gdb_index_command (const char *arg, int from_tty)
 				 _("Error while writing index for `%s': "),
 				 objfile_name (objfile));
 	    }
-	    }
+	}
 
     }
 }
-- 
2.35.3

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Post-commit review pointed out a few style issues in commit 8b9c08eddac
Tom> ("[gdb/symtab] Add name_of_main and language_of_main to the DWARF index").

Tom> Fix these.

Tom> Tested on x86_64-linux.

Thank you, this looks good.
Approved-By: Tom Tromey <tom@tromey.com>

I found a race in this code as well, I'm working on a fix for that though.

Tom

Re: [PATCH 2/2] [gdb/symtab] Fix two style issues in gdb/dwarf2/index-write.c

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> While reviewing gdb/dwarf2/index-write.c I noticed two style issues.
Tom> Fix these.

Tom> Tested on x86_64-linux.

Thanks.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

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

[Tom de Vries]

On 10/17/23 22:07, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Post-commit review pointed out a few style issues in commit 8b9c08eddac
> Tom> ("[gdb/symtab] Add name_of_main and language_of_main to the DWARF index").
> 
> Tom> Fix these.
> 
> Tom> Tested on x86_64-linux.
> 
> Thank you, this looks good.
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> I found a race in this code as well, I'm working on a fix for that though.

Thanks for the reviews, pushed.

I was curious, so I build gdb with thread sanitizer and ran the 
test-suite with target board cc-with-gdb-index, and ran into a 
write-write data race for:
...
   cu->per_cu->dw_lang = dw_lang;
...
for for instance test-case gdb.ada/catch_ex_std.exp.

I suppose that's the race you're referring to.

Thanks,
- Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I was curious, so I build gdb with thread sanitizer and ran the
Tom> test-suite with target board cc-with-gdb-index, and ran into a
Tom> write-write data race for:
Tom> ...
cu-> per_cu->dw_lang = dw_lang;
Tom> ...
Tom> for for instance test-case gdb.ada/catch_ex_std.exp.

Tom> I suppose that's the race you're referring to.

Yeah.  I sent the patch just now.

Tom

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

[Thiago Jung Bauermann]

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.

>    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.

>    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.

-- 
Thiago

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

[Tom de Vries]

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

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

[Thiago Jung Bauermann]

Tom de Vries <tdevries@suse.de> writes:

> 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;
> ...

Ah, thanks for pointing out. I should have dug into it.

>>>     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".

Indeed. I should brush up on my basic math. :-)

>> 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.

Thanks for the explanations!

-- 
Thiago