[review] Only make a nullterminated string if we need to
Commit Message
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
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.
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.
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.
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.
@@ -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);