[PATCH] gdb: add build mode to reverse order of _initialize function calls

[PATCH] gdb: add build mode to reverse order of _initialize function calls

[Simon Marchi]

We should not assume any order in which the _initialize functions get
called.  However, since their order comes from the order of source files
baked into the Makefile, it's possible for such dependencies to creep in
and for us not to notice.

I think that a good way to test for such dependencies is to reverse the
order of the calls.  Add an option in the Makefile to do this.  It can
be used as:

  $ make REVERSE_INIT_FILES=1

before init.c gets generated (if you want to re-generate init.c, either
do a "make clean" or remove stamp-init).

Building with this mode finds one dependency.  In symtab.c, we try to
add the alias "maintenance flush-symbol-cache" for the command
"maintenance flush symbol-cache".  The add_alias_cmd that takes a target
command name is used, which requires a comman lookup to be done.  At
this point, the "maintenance flush" prefix command hasn't been added
(it happens in maint.c), so the lookup fails.  The add_alias_cmd doesn't
add an alias and returns NULL.  That NULL is then passed to
deprecate_cmd, which causes a segfault.

Fix that by using the add_alias_cmd overload that accepts a
cmd_list_element as the target.  This doesn't care whether the prefix
has been created or not, since it doesn't need to do a command lookup.

gdb/ChangeLog:

	* Makefile.in: Add option to reverse INIT_FILES.
	* symtab.c (_initialize_symtab): Pass cmd_list_element to
	add_alias_cmd.

Change-Id: Id8ae7860c822da48455087ea50741c137f237922
---
 gdb/Makefile.in | 17 +++++++++++++++++
 gdb/symtab.c    |  6 +++---
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ba0cabb0216a..80de9334cd1f 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1850,6 +1850,23 @@ INIT_FILES = \
 	    $(filter-out init.o version.o %_S.o %_U.o,\
 	      $(COMMON_OBS))))
 
+# This mode can be used to reverse the order in which _initialize functions
+# are called, to verify that there is no hidden dependencies between them.
+#
+# Use with:
+#
+#   $ make REVERSE_INIT_FILES=1
+#
+# _before_ init.c is generated.
+
+ifeq ($(REVERSE_INIT_FILES),1)
+INIT_FILES := \
+	$(shell \
+	    for i in $(INIT_FILES); do \
+		echo $$i; \
+	    done | tac)
+endif
+
 init.c: stamp-init; @true
 stamp-init: $(INIT_FILES) config.status
 	@$(ECHO_INIT_C) echo "Making init.c"
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9555f94707de..8ad319266772 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -6897,12 +6897,12 @@ If zero then the symbol cache is disabled."),
 	   _("Print symbol cache statistics for each program space."),
 	   &maintenanceprintlist);
 
-  add_cmd ("symbol-cache", class_maintenance,
+  c = add_cmd ("symbol-cache", class_maintenance,
 	   maintenance_flush_symbol_cache,
 	   _("Flush the symbol cache for each program space."),
 	   &maintenanceflushlist);
-  c = add_alias_cmd ("flush-symbol-cache", "flush symbol-cache",
-		     class_maintenance, 0, &maintenancelist);
+  c = add_alias_cmd ("flush-symbol-cache", c, class_maintenance, 0,
+		     &maintenancelist);
   deprecate_cmd (c, "maintenancelist flush symbol-cache");
 
   gdb::observers::executable_changed.attach (symtab_observer_executable_changed,
-- 
2.31.1

Re: [PATCH] gdb: add build mode to reverse order of _initialize function calls

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-17 14:00:03 -0400]:

> We should not assume any order in which the _initialize functions get
> called.  However, since their order comes from the order of source files
> baked into the Makefile, it's possible for such dependencies to creep in
> and for us not to notice.
> 
> I think that a good way to test for such dependencies is to reverse the
> order of the calls.  Add an option in the Makefile to do this.  It can
> be used as:
> 
>   $ make REVERSE_INIT_FILES=1
> 
> before init.c gets generated (if you want to re-generate init.c, either
> do a "make clean" or remove stamp-init).

Instead of making this a build option, did you consider modifying
initialize_all_files to something like this totally untested code:

  void
  initialize_all_files (bool reverse_p)
  {
    std::vector<void (*) (void)> func = {
      _initialize_arc_linux_tdep,
      _initialize_arc_tdep,
      _initialize_arm_fbsd_tdep,
      _initialize_arm_linux_tdep,
      _initialize_arm_netbsd_tdep,
      _initialize_armobsd_tdep,
      ... etc ...
   };

   if (reverse_p)
     std::reverse(func.begin(), func.end());

   for (auto f : func)
     f ();
  }

then all you'd need is to add a new command line flag which controls
reverse_p.

The benefit as I see it is that (a) we can add a test that is run in
every run of the testsuite that starts GDB with this new command line
flag, and does a few basic tests, and (b) we can easily pass the new
command line flag to the testsuite from the command line, and so run
_all_ tests with the reverse init order.

Just a thought.

Andrew



> 
> Building with this mode finds one dependency.  In symtab.c, we try to
> add the alias "maintenance flush-symbol-cache" for the command
> "maintenance flush symbol-cache".  The add_alias_cmd that takes a target
> command name is used, which requires a comman lookup to be done.  At
> this point, the "maintenance flush" prefix command hasn't been added
> (it happens in maint.c), so the lookup fails.  The add_alias_cmd doesn't
> add an alias and returns NULL.  That NULL is then passed to
> deprecate_cmd, which causes a segfault.
> 
> Fix that by using the add_alias_cmd overload that accepts a
> cmd_list_element as the target.  This doesn't care whether the prefix
> has been created or not, since it doesn't need to do a command lookup.
> 
> gdb/ChangeLog:
> 
> 	* Makefile.in: Add option to reverse INIT_FILES.
> 	* symtab.c (_initialize_symtab): Pass cmd_list_element to
> 	add_alias_cmd.
> 
> Change-Id: Id8ae7860c822da48455087ea50741c137f237922
> ---
>  gdb/Makefile.in | 17 +++++++++++++++++
>  gdb/symtab.c    |  6 +++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index ba0cabb0216a..80de9334cd1f 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1850,6 +1850,23 @@ INIT_FILES = \
>  	    $(filter-out init.o version.o %_S.o %_U.o,\
>  	      $(COMMON_OBS))))
>  
> +# This mode can be used to reverse the order in which _initialize functions
> +# are called, to verify that there is no hidden dependencies between them.
> +#
> +# Use with:
> +#
> +#   $ make REVERSE_INIT_FILES=1
> +#
> +# _before_ init.c is generated.
> +
> +ifeq ($(REVERSE_INIT_FILES),1)
> +INIT_FILES := \
> +	$(shell \
> +	    for i in $(INIT_FILES); do \
> +		echo $$i; \
> +	    done | tac)
> +endif
> +
>  init.c: stamp-init; @true
>  stamp-init: $(INIT_FILES) config.status
>  	@$(ECHO_INIT_C) echo "Making init.c"
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 9555f94707de..8ad319266772 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -6897,12 +6897,12 @@ If zero then the symbol cache is disabled."),
>  	   _("Print symbol cache statistics for each program space."),
>  	   &maintenanceprintlist);
>  
> -  add_cmd ("symbol-cache", class_maintenance,
> +  c = add_cmd ("symbol-cache", class_maintenance,
>  	   maintenance_flush_symbol_cache,
>  	   _("Flush the symbol cache for each program space."),
>  	   &maintenanceflushlist);
> -  c = add_alias_cmd ("flush-symbol-cache", "flush symbol-cache",
> -		     class_maintenance, 0, &maintenancelist);
> +  c = add_alias_cmd ("flush-symbol-cache", c, class_maintenance, 0,
> +		     &maintenancelist);
>    deprecate_cmd (c, "maintenancelist flush symbol-cache");
>  
>    gdb::observers::executable_changed.attach (symtab_observer_executable_changed,
> -- 
> 2.31.1
> 

Re: [PATCH] gdb: add build mode to reverse order of _initialize function calls

[Simon Marchi]

On 2021-05-17 3:26 p.m., Andrew Burgess wrote:
> * Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-17 14:00:03 -0400]:
> 
>> We should not assume any order in which the _initialize functions get
>> called.  However, since their order comes from the order of source files
>> baked into the Makefile, it's possible for such dependencies to creep in
>> and for us not to notice.
>>
>> I think that a good way to test for such dependencies is to reverse the
>> order of the calls.  Add an option in the Makefile to do this.  It can
>> be used as:
>>
>>   $ make REVERSE_INIT_FILES=1
>>
>> before init.c gets generated (if you want to re-generate init.c, either
>> do a "make clean" or remove stamp-init).
> 
> Instead of making this a build option, did you consider modifying
> initialize_all_files to something like this totally untested code:
> 
>   void
>   initialize_all_files (bool reverse_p)
>   {
>     std::vector<void (*) (void)> func = {
>       _initialize_arc_linux_tdep,
>       _initialize_arc_tdep,
>       _initialize_arm_fbsd_tdep,
>       _initialize_arm_linux_tdep,
>       _initialize_arm_netbsd_tdep,
>       _initialize_armobsd_tdep,
>       ... etc ...
>    };
> 
>    if (reverse_p)
>      std::reverse(func.begin(), func.end());
> 
>    for (auto f : func)
>      f ();
>   }
> 
> then all you'd need is to add a new command line flag which controls
> reverse_p.
> 
> The benefit as I see it is that (a) we can add a test that is run in
> every run of the testsuite that starts GDB with this new command line
> flag, and does a few basic tests, and (b) we can easily pass the new
> command line flag to the testsuite from the command line, and so run
> _all_ tests with the reverse init order.
> 
> Just a thought.

That sounds much better indeed!

I'll give it a shot eventually.

Simon