[PATCH 1/2] gdb/linux-tdep: clearer variable names in linux_get_siginfo_type

[PATCH 1/2] gdb/linux-tdep: clearer variable names in linux_get_siginfo_type

[Simon Marchi]

Instead of reusing the single variable `type`, use distinct variables
for the various types created.  I find it more readable this way, it's
easier to track what's being added to what.

Do other cleanups at the same time, like NULL -> nullptr, removing some
`struct` keywords, and moving variable declarations to the point of
first use.

Change-Id: I846df0d4ad2208b768465c14abdc5422cc16bd07
---
 gdb/linux-tdep.c | 154 +++++++++++++++++++++++++----------------------
 1 file changed, 83 insertions(+), 71 deletions(-)

diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index 23e43ba5c5f9..fa9e5cb6f4d9 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -268,58 +268,54 @@ get_linux_inferior_data (inferior *inf)
 
 /* Implementation of gdbarch_get_siginfo_type.  */
 
-static struct type *
+static type *
 linux_get_siginfo_type (struct gdbarch *gdbarch)
 {
-  struct linux_gdbarch_data *linux_gdbarch_data;
-  struct type *void_ptr_type;
-  struct type *uid_type, *pid_type;
-  struct type *sigval_type, *clock_type;
-  struct type *siginfo_type, *sifields_type;
-  struct type *type;
+  linux_gdbarch_data *linux_gdbarch_data = get_linux_gdbarch_data (gdbarch);
 
-  linux_gdbarch_data = get_linux_gdbarch_data (gdbarch);
-  if (linux_gdbarch_data->siginfo_type != NULL)
+  if (linux_gdbarch_data->siginfo_type != nullptr)
     return linux_gdbarch_data->siginfo_type;
 
   type_allocator alloc (gdbarch);
 
   const struct builtin_type *builtin_types = builtin_type (gdbarch);
-  struct type *int_type = builtin_types->builtin_int;
-  struct type *uint_type = builtin_types->builtin_unsigned_int;
-  struct type *long_type = builtin_types->builtin_long;
-
-  void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
+  type *int_type = builtin_types->builtin_int;
+  type *uint_type = builtin_types->builtin_unsigned_int;
+  type *long_type = builtin_types->builtin_long;
+  type *void_ptr_type
+    = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
 
   /* sival_t */
-  sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
-  sigval_type->set_name (xstrdup ("sigval_t"));
-  append_composite_type_field (sigval_type, "sival_int", int_type);
-  append_composite_type_field (sigval_type, "sival_ptr", void_ptr_type);
+  type *sigval_union_type = arch_composite_type (gdbarch, nullptr,
+						 TYPE_CODE_UNION);
+  sigval_union_type->set_name (xstrdup ("sigval_t"));
+  append_composite_type_field (sigval_union_type, "sival_int", int_type);
+  append_composite_type_field (sigval_union_type, "sival_ptr", void_ptr_type);
 
   /* __pid_t */
-  pid_type = alloc.new_type (TYPE_CODE_TYPEDEF,
-			     int_type->length () * TARGET_CHAR_BIT,
-			     "__pid_t");
+  type *pid_type = alloc.new_type (TYPE_CODE_TYPEDEF,
+				   int_type->length () * TARGET_CHAR_BIT,
+				   "__pid_t");
   pid_type->set_target_type (int_type);
   pid_type->set_target_is_stub (true);
 
   /* __uid_t */
-  uid_type = alloc.new_type (TYPE_CODE_TYPEDEF,
-			     uint_type->length () * TARGET_CHAR_BIT,
-			     "__uid_t");
+  type *uid_type = alloc.new_type (TYPE_CODE_TYPEDEF,
+				   uint_type->length () * TARGET_CHAR_BIT,
+				   "__uid_t");
   uid_type->set_target_type (uint_type);
   uid_type->set_target_is_stub (true);
 
   /* __clock_t */
-  clock_type = alloc.new_type (TYPE_CODE_TYPEDEF,
-			       long_type->length () * TARGET_CHAR_BIT,
-			       "__clock_t");
+  type *clock_type = alloc.new_type (TYPE_CODE_TYPEDEF,
+				     long_type->length () * TARGET_CHAR_BIT,
+				     "__clock_t");
   clock_type->set_target_type (long_type);
   clock_type->set_target_is_stub (true);
 
   /* _sifields */
-  sifields_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
+  type *sifields_union_type = arch_composite_type (gdbarch, nullptr,
+						   TYPE_CODE_UNION);
 
   {
     const int si_max_size = 128;
@@ -331,70 +327,86 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
       si_pad_size = (si_max_size / size_of_int) - 4;
     else
       si_pad_size = (si_max_size / size_of_int) - 3;
-    append_composite_type_field (sifields_type, "_pad",
+
+    append_composite_type_field (sifields_union_type, "_pad",
 				 init_vector_type (int_type, si_pad_size));
   }
 
   /* _kill */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_pid", pid_type);
-  append_composite_type_field (type, "si_uid", uid_type);
-  append_composite_type_field (sifields_type, "_kill", type);
+  type *kill_struct_type = arch_composite_type (gdbarch, nullptr,
+						TYPE_CODE_STRUCT);
+  append_composite_type_field (kill_struct_type, "si_pid", pid_type);
+  append_composite_type_field (kill_struct_type, "si_uid", uid_type);
+  append_composite_type_field (sifields_union_type, "_kill", kill_struct_type);
 
   /* _timer */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_tid", int_type);
-  append_composite_type_field (type, "si_overrun", int_type);
-  append_composite_type_field (type, "si_sigval", sigval_type);
-  append_composite_type_field (sifields_type, "_timer", type);
+  type *timer_struct_type = arch_composite_type (gdbarch, nullptr,
+						 TYPE_CODE_STRUCT);
+  append_composite_type_field (timer_struct_type, "si_tid", int_type);
+  append_composite_type_field (timer_struct_type, "si_overrun", int_type);
+  append_composite_type_field (timer_struct_type, "si_sigval",
+			       sigval_union_type);
+  append_composite_type_field (sifields_union_type, "_timer",
+			       timer_struct_type);
 
   /* _rt */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_pid", pid_type);
-  append_composite_type_field (type, "si_uid", uid_type);
-  append_composite_type_field (type, "si_sigval", sigval_type);
-  append_composite_type_field (sifields_type, "_rt", type);
+  type *rt_struct_type = arch_composite_type (gdbarch, nullptr,
+					      TYPE_CODE_STRUCT);
+  append_composite_type_field (rt_struct_type, "si_pid", pid_type);
+  append_composite_type_field (rt_struct_type, "si_uid", uid_type);
+  append_composite_type_field (rt_struct_type, "si_sigval", sigval_union_type);
+  append_composite_type_field (sifields_union_type, "_rt", rt_struct_type);
 
   /* _sigchld */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_pid", pid_type);
-  append_composite_type_field (type, "si_uid", uid_type);
-  append_composite_type_field (type, "si_status", int_type);
-  append_composite_type_field (type, "si_utime", clock_type);
-  append_composite_type_field (type, "si_stime", clock_type);
-  append_composite_type_field (sifields_type, "_sigchld", type);
+  type *sigchld_struct_type = arch_composite_type (gdbarch, nullptr,
+						   TYPE_CODE_STRUCT);
+  append_composite_type_field (sigchld_struct_type, "si_pid", pid_type);
+  append_composite_type_field (sigchld_struct_type, "si_uid", uid_type);
+  append_composite_type_field (sigchld_struct_type, "si_status", int_type);
+  append_composite_type_field (sigchld_struct_type, "si_utime", clock_type);
+  append_composite_type_field (sigchld_struct_type, "si_stime", clock_type);
+  append_composite_type_field (sifields_union_type, "_sigchld",
+			       sigchld_struct_type);
 
   /* _sigfault */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_addr", void_ptr_type);
-  append_composite_type_field (sifields_type, "_sigfault", type);
+  type *sigfault_struct_type = arch_composite_type (gdbarch, nullptr,
+						    TYPE_CODE_STRUCT);
+  append_composite_type_field (sigfault_struct_type, "si_addr", void_ptr_type);
+  append_composite_type_field (sifields_union_type, "_sigfault",
+			       sigfault_struct_type);
 
   /* _sigpoll */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "si_band", long_type);
-  append_composite_type_field (type, "si_fd", int_type);
-  append_composite_type_field (sifields_type, "_sigpoll", type);
+  type *sigpoll_struct_type = arch_composite_type (gdbarch, NULL,
+						   TYPE_CODE_STRUCT);
+  append_composite_type_field (sigpoll_struct_type, "si_band", long_type);
+  append_composite_type_field (sigpoll_struct_type, "si_fd", int_type);
+  append_composite_type_field (sifields_union_type, "_sigpoll",
+			       sigpoll_struct_type);
 
   /* _sigsys */
-  type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  append_composite_type_field (type, "_call_addr", void_ptr_type);
-  append_composite_type_field (type, "_syscall", int_type);
-  append_composite_type_field (type, "_arch", uint_type);
-  append_composite_type_field (sifields_type, "_sigsys", type);
+  type *sigsys_struct_type = arch_composite_type (gdbarch, nullptr,
+						  TYPE_CODE_STRUCT);
+  append_composite_type_field (sigsys_struct_type, "_call_addr",
+			       void_ptr_type);
+  append_composite_type_field (sigsys_struct_type, "_syscall", int_type);
+  append_composite_type_field (sigsys_struct_type, "_arch", uint_type);
+  append_composite_type_field (sifields_union_type, "_sigsys",
+			       sigsys_struct_type);
 
   /* struct siginfo */
-  siginfo_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  siginfo_type->set_name (xstrdup ("siginfo"));
-  append_composite_type_field (siginfo_type, "si_signo", int_type);
-  append_composite_type_field (siginfo_type, "si_errno", int_type);
-  append_composite_type_field (siginfo_type, "si_code", int_type);
-  append_composite_type_field_aligned (siginfo_type,
-				       "_sifields", sifields_type,
+  type *siginfo_struct_type = arch_composite_type (gdbarch, nullptr,
+						   TYPE_CODE_STRUCT);
+  siginfo_struct_type->set_name (xstrdup ("siginfo"));
+  append_composite_type_field (siginfo_struct_type, "si_signo", int_type);
+  append_composite_type_field (siginfo_struct_type, "si_errno", int_type);
+  append_composite_type_field (siginfo_struct_type, "si_code", int_type);
+  append_composite_type_field_aligned (siginfo_struct_type, "_sifields",
+				       sifields_union_type,
 				       long_type->length ());
 
-  linux_gdbarch_data->siginfo_type = siginfo_type;
+  linux_gdbarch_data->siginfo_type = siginfo_struct_type;
 
-  return siginfo_type;
+  return siginfo_struct_type;
 }
 
 /* Return true if the target is running on uClinux instead of normal

base-commit: 1686e21559a7812ebbc05f57372cc30880768bf7
-- 
2.55.0

[PATCH 2/2] gdb: pass type name directly to arch_composite_type

[Simon Marchi]

There are a few spots that call arch_composite_type followed by set_name
on the created type:

  sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
  sigval_type->set_name (xstrdup ("sigval"));

First, it's not necessary to duplicate the string, we can pass the
literal string directly, and its lifetime will be appropriate.  Second,
we can pass the name directly to arch_composite_type, instead of doing a
separate set_name call.  So, replace the above with:

  sigval_type = arch_composite_type (gdbarch, "sigval", TYPE_CODE_UNION);

Tested on Linux by running the gdb.*/*siginfo*.exp tests.

Change-Id: I7779e2156ca42212623dea8de709887acc40d242
---
 gdb/fbsd-tdep.c    |  7 +++----
 gdb/linux-tdep.c   |  6 ++----
 gdb/netbsd-tdep.c  | 16 ++++++++--------
 gdb/windows-tdep.c | 16 ++++++----------
 4 files changed, 19 insertions(+), 26 deletions(-)

diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index 419f935ea72f..0bb63b8c199d 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -1590,8 +1590,7 @@ fbsd_get_siginfo_type (struct gdbarch *gdbarch)
   void_ptr_type = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
 
   /* union sigval */
-  sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
-  sigval_type->set_name (xstrdup ("sigval"));
+  sigval_type = arch_composite_type (gdbarch, "sigval", TYPE_CODE_UNION);
   append_composite_type_field (sigval_type, "sival_int", int_type);
   append_composite_type_field (sigval_type, "sival_ptr", void_ptr_type);
 
@@ -1641,8 +1640,8 @@ fbsd_get_siginfo_type (struct gdbarch *gdbarch)
   append_composite_type_field (reason_type, "__spare__", type);
 
   /* struct siginfo */
-  siginfo_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  siginfo_type->set_name (xstrdup ("siginfo"));
+  siginfo_type = arch_composite_type (gdbarch, "siginfo",
+				      TYPE_CODE_STRUCT);
   append_composite_type_field (siginfo_type, "si_signo", int_type);
   append_composite_type_field (siginfo_type, "si_errno", int_type);
   append_composite_type_field (siginfo_type, "si_code", int_type);
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index fa9e5cb6f4d9..de2e5aba313c 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -286,9 +286,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
     = lookup_pointer_type (builtin_type (gdbarch)->builtin_void);
 
   /* sival_t */
-  type *sigval_union_type = arch_composite_type (gdbarch, nullptr,
+  type *sigval_union_type = arch_composite_type (gdbarch, "sigval_t",
 						 TYPE_CODE_UNION);
-  sigval_union_type->set_name (xstrdup ("sigval_t"));
   append_composite_type_field (sigval_union_type, "sival_int", int_type);
   append_composite_type_field (sigval_union_type, "sival_ptr", void_ptr_type);
 
@@ -394,9 +393,8 @@ linux_get_siginfo_type (struct gdbarch *gdbarch)
 			       sigsys_struct_type);
 
   /* struct siginfo */
-  type *siginfo_struct_type = arch_composite_type (gdbarch, nullptr,
+  type *siginfo_struct_type = arch_composite_type (gdbarch, "siginfo",
 						   TYPE_CODE_STRUCT);
-  siginfo_struct_type->set_name (xstrdup ("siginfo"));
   append_composite_type_field (siginfo_struct_type, "si_signo", int_type);
   append_composite_type_field (siginfo_struct_type, "si_errno", int_type);
   append_composite_type_field (siginfo_struct_type, "si_code", int_type);
diff --git a/gdb/netbsd-tdep.c b/gdb/netbsd-tdep.c
index e369622ae7f7..1c0569873091 100644
--- a/gdb/netbsd-tdep.c
+++ b/gdb/netbsd-tdep.c
@@ -419,14 +419,14 @@ nbsd_get_siginfo_type (struct gdbarch *gdbarch)
   lwpid_type->set_target_type (int32_type);
 
   /* union sigval */
-  type *sigval_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
-  sigval_type->set_name (gdbarch_obstack_strdup (gdbarch, "sigval"));
+  type *sigval_type = arch_composite_type (gdbarch, "sigval",
+					   TYPE_CODE_UNION);
   append_composite_type_field (sigval_type, "sival_int", int_type);
   append_composite_type_field (sigval_type, "sival_ptr", void_ptr_type);
 
   /* union _option */
-  type *option_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
-  option_type->set_name (gdbarch_obstack_strdup (gdbarch, "_option"));
+  type *option_type = arch_composite_type (gdbarch, "_option",
+					   TYPE_CODE_UNION);
   append_composite_type_field (option_type, "_pe_other_pid", pid_type);
   append_composite_type_field (option_type, "_pe_lwp", lwpid_type);
 
@@ -480,8 +480,8 @@ nbsd_get_siginfo_type (struct gdbarch *gdbarch)
   append_composite_type_field (reason_type, "_ptrace_state", t);
 
   /* struct _ksiginfo */
-  type *ksiginfo_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  ksiginfo_type->set_name (gdbarch_obstack_strdup (gdbarch, "_ksiginfo"));
+  type *ksiginfo_type = arch_composite_type (gdbarch, "_ksiginfo",
+					     TYPE_CODE_STRUCT);
   append_composite_type_field (ksiginfo_type, "_signo", int_type);
   append_composite_type_field (ksiginfo_type, "_code", int_type);
   append_composite_type_field (ksiginfo_type, "_errno", int_type);
@@ -490,8 +490,8 @@ nbsd_get_siginfo_type (struct gdbarch *gdbarch)
   append_composite_type_field (ksiginfo_type, "_reason", reason_type);
 
   /* union siginfo */
-  type *siginfo_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION);
-  siginfo_type->set_name (gdbarch_obstack_strdup (gdbarch, "siginfo"));
+  type *siginfo_type = arch_composite_type (gdbarch, "siginfo",
+					    TYPE_CODE_UNION);
   append_composite_type_field (siginfo_type, "si_pad",
 			       init_vector_type (char_type, 128));
   append_composite_type_field (siginfo_type, "_info", ksiginfo_type);
diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index c0a743914b1c..6e83c8773c2d 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -225,8 +225,7 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
 
   /* list entry */
 
-  list_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  list_type->set_name (xstrdup ("list"));
+  list_type = arch_composite_type (gdbarch, "list", TYPE_CODE_STRUCT);
 
   module_list_ptr_type = void_ptr_type;
 
@@ -237,8 +236,7 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
 
   /* Structured Exception Handler */
 
-  seh_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  seh_type->set_name (xstrdup ("seh"));
+  seh_type = arch_composite_type (gdbarch, "seh", TYPE_CODE_STRUCT);
 
   seh_ptr_type = alloc.new_type (TYPE_CODE_PTR,
 				 void_ptr_type->length () * TARGET_CHAR_BIT,
@@ -250,8 +248,8 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
 			       builtin_type (gdbarch)->builtin_func_ptr);
 
   /* struct _PEB_LDR_DATA */
-  peb_ldr_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  peb_ldr_type->set_name (xstrdup ("peb_ldr_data"));
+  peb_ldr_type = arch_composite_type (gdbarch, "peb_ldr_data",
+				      TYPE_CODE_STRUCT);
 
   append_composite_type_field (peb_ldr_type, "length", dword32_type);
   append_composite_type_field (peb_ldr_type, "initialized", dword32_type);
@@ -319,8 +317,7 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
 
 
   /* struct process environment block */
-  peb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  peb_type->set_name (xstrdup ("peb"));
+  peb_type = arch_composite_type (gdbarch, "peb", TYPE_CODE_STRUCT);
 
   /* First bytes contain several flags.  */
   append_composite_type_field (peb_type, "flags", dword_ptr_type);
@@ -338,8 +335,7 @@ windows_get_tlb_type (struct gdbarch *gdbarch)
 
 
   /* struct thread information block */
-  tib_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT);
-  tib_type->set_name (xstrdup ("tib"));
+  tib_type = arch_composite_type (gdbarch, "tib", TYPE_CODE_STRUCT);
 
   /* uint32_t current_seh;			%fs:0x0000 */
   append_composite_type_field (tib_type, "current_seh", seh_ptr_type);
-- 
2.55.0

Re: [PATCH 1/2] gdb/linux-tdep: clearer variable names in linux_get_siginfo_type

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Instead of reusing the single variable `type`, use distinct variables
Simon> for the various types created.  I find it more readable this way, it's
Simon> easier to track what's being added to what.

Simon> Do other cleanups at the same time, like NULL -> nullptr, removing some
Simon> `struct` keywords, and moving variable declarations to the point of
Simon> first use.

Looks good, thanks.
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 2/2] gdb: pass type name directly to arch_composite_type

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> First, it's not necessary to duplicate the string, we can pass the
Simon> literal string directly, and its lifetime will be appropriate.  Second,
Simon> we can pass the name directly to arch_composite_type, instead of doing a
Simon> separate set_name call.  So, replace the above with:

Simon>   sigval_type = arch_composite_type (gdbarch, "sigval", TYPE_CODE_UNION);

Thanks.  One tiny nit below.

Simon>    /* sival_t */
Simon> -  type *sigval_union_type = arch_composite_type (gdbarch, nullptr,
Simon> +  type *sigval_union_type = arch_composite_type (gdbarch, "sigval_t",
Simon>  						 TYPE_CODE_UNION);

The comment says "sival_t" but the type says "sigval_t".
The Linux headers say the latter, so the comment is wrong.

Personally I think this comment isn't so useful and could be removed,
but fixing it would also be fine.

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

Tom