[review] Only make a nullterminated string if we need to

Message ID gerrit.1571783424000.I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2@gnutoolchain-gerrit.osci.io
State New
Headers show
Series
  • [review] Only make a nullterminated string if we need to
Related show

Commit Message

Simon Marchi (Code Review) Oct. 22, 2019, 10:30 p.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222
......................................................................

Only make a nullterminated string if we need to

As of 7bb43059820c5febb4509b15202a93efde442bc6, we no longer need
a nullterminated linkage_name to look up the entry in the hash table.

So this patch makes it so we only make the copy if the entry was
not found.

gdb/ChangeLog:

2019-10-22  Christian Biesinger  <cbiesinger@google.com>

	* symtab.c (symbol_set_names): Only make a nullterminated copy of
	linkage_name if the entry was not found and we need to demangle.

Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2
---
M gdb/symtab.c
1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Simon Marchi (Code Review) Oct. 25, 2019, 5:59 p.m. | #1
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222
......................................................................


Patch Set 1:

Thanks for the patch.

The patch moves the `linkage_name_copy` code a bit lower.
But, I think this copy is actually no longer needed at all, and
so the code can be removed.  The only real (non-flag) use of
linkage_name_copy is to again copy:

	  strcpy (mangled_ptr, linkage_name_copy);

... but this could be replaced by a memcpy and then a store of
the trailing \0.

Wouldn't this change also re-enable the string_view change?
It seems that way to me, but I wonder if I am missing something here.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2
Gerrit-Change-Number: 222
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 25 Oct 2019 17:59:13 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Oct. 25, 2019, 6:03 p.m. | #2
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222
......................................................................


Patch Set 1:

> Patch Set 1:

> 

> Thanks for the patch.

> 

> The patch moves the `linkage_name_copy` code a bit lower.

> But, I think this copy is actually no longer needed at all, and

> so the code can be removed.  The only real (non-flag) use of

> linkage_name_copy is to again copy:

> 

> 	  strcpy (mangled_ptr, linkage_name_copy);

> 

> ... but this could be replaced by a memcpy and then a store of

> the trailing \0.

> 

> Wouldn't this change also re-enable the string_view change?

> It seems that way to me, but I wonder if I am missing something here.


Unfortunately it is also used in the call to symbol_find_demangled_name, and that can't be changed without changing bfd API; I haven't investigated that but I suspect it may be difficult.

You are correct as far as that strcpy goes, of course.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2
Gerrit-Change-Number: 222
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 25 Oct 2019 18:03:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Oct. 25, 2019, 8:07 p.m. | #3
Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222
......................................................................


Patch Set 2:

> Patch Set 1:

> 

> Thanks for the patch.

> 

> The patch moves the `linkage_name_copy` code a bit lower.

> But, I think this copy is actually no longer needed at all, and

> so the code can be removed.  The only real (non-flag) use of

> linkage_name_copy is to again copy:

> 

> 	  strcpy (mangled_ptr, linkage_name_copy);

> 

> ... but this could be replaced by a memcpy and then a store of

> the trailing \0.

> 

> Wouldn't this change also re-enable the string_view change?

> It seems that way to me, but I wonder if I am missing something here.


I did update the patch to use the memcpy.

There are still two issues blocking the string_view change:
- The demangle call mentioned in my previous comment. This could be addressed by just always doing alloca + memcpy, which should be relatively cheap compared to the actual demangling, I'd think
- Even if copy_name==false, this function will still copy the name if it is not nullterminated (it has to, because general_symbol_info expects a nullterminated string). This could be fixed by adding a size to general_symbol_info. That should be doable, and I'll look into it

However, I think this patch is good even before solving those two issues.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2
Gerrit-Change-Number: 222
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Fri, 25 Oct 2019 20:07:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Simon Marchi (Code Review) Oct. 29, 2019, 7:09 p.m. | #4
Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/222
......................................................................


Patch Set 4: Code-Review+2

Thank you.  This is ok.

I suddenly realized that maybe storing a string_view is confusing
because, in reality, the stored strings must always be \0-terminated,
because that's how they are used in the rest of the code (via the
symbol names).  But it seems like if there was a problem, this patch
must fix it.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I183302e1f51483ff6dff0fd5c3b0f32f0f04a5d2
Gerrit-Change-Number: 222
Gerrit-PatchSet: 4
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Luis Machado <luis.machado@linaro.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 19:09:56 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 0a87fec..787c4b6 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -829,8 +829,6 @@ 
 		  struct objfile_per_bfd_storage *per_bfd)
 {
   struct demangled_name_entry **slot;
-  /* A 0-terminated copy of the linkage name.  */
-  const char *linkage_name_copy;
 
   if (gsymbol->language == language_ada)
     {
@@ -855,20 +853,7 @@ 
   if (per_bfd->demangled_names_hash == NULL)
     create_demangled_names_hash (per_bfd);
 
-  if (linkage_name[len] != '\0')
-    {
-      char *alloc_name;
-
-      alloc_name = (char *) alloca (len + 1);
-      memcpy (alloc_name, linkage_name, len);
-      alloc_name[len] = '\0';
-
-      linkage_name_copy = alloc_name;
-    }
-  else
-    linkage_name_copy = linkage_name;
-
-  struct demangled_name_entry entry (gdb::string_view (linkage_name_copy, len));
+  struct demangled_name_entry entry (gdb::string_view (linkage_name, len));
   slot = ((struct demangled_name_entry **)
 	  htab_find_slot (per_bfd->demangled_names_hash.get (),
 			  &entry, INSERT));
@@ -880,6 +865,21 @@ 
       || (gsymbol->language == language_go
 	  && (*slot)->demangled[0] == '\0'))
     {
+      /* A 0-terminated copy of the linkage name.  */
+      const char *linkage_name_copy;
+      if (linkage_name[len] != '\0')
+	{
+	  char *alloc_name;
+
+	  alloc_name = (char *) alloca (len + 1);
+	  memcpy (alloc_name, linkage_name, len);
+	  alloc_name[len] = '\0';
+
+	  linkage_name_copy = alloc_name;
+	}
+      else
+	linkage_name_copy = linkage_name;
+
       char *demangled_name_ptr
 	= symbol_find_demangled_name (gsymbol, linkage_name_copy);
       gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);