[2/2] Use gdbarch obstack to allocate the TYPE_NAME string in arch_type
Commit Message
Since the type whose name is being set is now being allocated on the
gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
This reduces the number of individual valgrind warnings for the command
"gdb gdb" from ~300 to ~150.
Tested on x86_64-unknown-linux-gnu.
[ I have a few more patches on top of these that together bring the total
number of valgrind warnings for the command "gdb gdb" down to ~30
but they are more controversial than these two, and if these aren't OK
then the rest definitely aren't OK. ]
gdb/ChangeLog:
* gdbarch.h (gdbarch_obstack_strdup): Declare.
* gdbarch.c (gdbarch_obstack_strdup): Define.
* gdbtypes.c (arch_type): Use it.
---
gdb/gdbarch.c | 10 ++++++++++
gdb/gdbarch.h | 5 +++++
gdb/gdbtypes.c | 2 +-
3 files changed, 16 insertions(+), 1 deletion(-)
Comments
On 06/30/2015 03:28 AM, Patrick Palka wrote:
> Since the type whose name is being set is now being allocated on the
> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
> This reduces the number of individual valgrind warnings for the command
> "gdb gdb" from ~300 to ~150.
>
> Tested on x86_64-unknown-linux-gnu.
>
> [ I have a few more patches on top of these that together bring the total
> number of valgrind warnings for the command "gdb gdb" down to ~30
> but they are more controversial than these two, and if these aren't OK
> then the rest definitely aren't OK. ]
Both patches look fine to me. If this blows up for some reason, I guess
we'll take the opportunity to add a comment explaining why these must go
on the heap. :-)
Thanks,
Pedro Alves
On Tue, Jun 30, 2015 at 5:36 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/30/2015 03:28 AM, Patrick Palka wrote:
>> Since the type whose name is being set is now being allocated on the
>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>> This reduces the number of individual valgrind warnings for the command
>> "gdb gdb" from ~300 to ~150.
>>
>> Tested on x86_64-unknown-linux-gnu.
>>
>> [ I have a few more patches on top of these that together bring the total
>> number of valgrind warnings for the command "gdb gdb" down to ~30
>> but they are more controversial than these two, and if these aren't OK
>> then the rest definitely aren't OK. ]
>
> Both patches look fine to me. If this blows up for some reason, I guess
> we'll take the opportunity to add a comment explaining why these must go
> on the heap. :-)
Heh, sounds good. I will hold off committing these patches at least
until 7.10 gets released to avoid unnecessary breakage.
On Tue, Jun 30, 2015 at 4:04 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Jun 30, 2015 at 5:36 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/30/2015 03:28 AM, Patrick Palka wrote:
>>> Since the type whose name is being set is now being allocated on the
>>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>>> This reduces the number of individual valgrind warnings for the command
>>> "gdb gdb" from ~300 to ~150.
>>>
>>> Tested on x86_64-unknown-linux-gnu.
>>>
>>> [ I have a few more patches on top of these that together bring the total
>>> number of valgrind warnings for the command "gdb gdb" down to ~30
>>> but they are more controversial than these two, and if these aren't OK
>>> then the rest definitely aren't OK. ]
>>
>> Both patches look fine to me. If this blows up for some reason, I guess
>> we'll take the opportunity to add a comment explaining why these must go
>> on the heap. :-)
>
> Heh, sounds good. I will hold off committing these patches at least
> until 7.10 gets released to avoid unnecessary breakage.
Committed. Let's see if anything breaks.
Patrick Palka <patrick@parcs.ath.cx> writes:
> Since the type whose name is being set is now being allocated on the
> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
> This reduces the number of individual valgrind warnings for the command
> "gdb gdb" from ~300 to ~150.
>
> Tested on x86_64-unknown-linux-gnu.
>
> [ I have a few more patches on top of these that together bring the total
> number of valgrind warnings for the command "gdb gdb" down to ~30
> but they are more controversial than these two, and if these aren't OK
> then the rest definitely aren't OK. ]
>
> gdb/ChangeLog:
>
> * gdbarch.h (gdbarch_obstack_strdup): Declare.
> * gdbarch.c (gdbarch_obstack_strdup): Define.
> * gdbtypes.c (arch_type): Use it.
Hi.
A couple of comments.
1) gdbarch.[ch] are machine generated.
IIUC, you have to edit gdbarch.sh and then run it to regenerate gdbarch.[ch].
2) I would have done this slightly differently.
If the obstack API doesn't provide a strdup functionality,
I wouldn't insist on trying to add it there. But I would like
to see it added to gdb in an application-independent way.
(make it non-gdbarch specific). obstack_strdup sounds sufficiently
useful and generic enough. If one wants to add a
gdbarch_obstack_strdup wrapper on top of that, fine by me.
IOW, add obstack_strdup to gdb_obstack.[ch].
On Sat, Aug 29, 2015 at 2:19 PM, Doug Evans <xdje42@gmail.com> wrote:
> Patrick Palka <patrick@parcs.ath.cx> writes:
>> Since the type whose name is being set is now being allocated on the
>> gdbarch obstack, we should allocate its TYPE_NAME on the obstack too.
>> This reduces the number of individual valgrind warnings for the command
>> "gdb gdb" from ~300 to ~150.
>>
>> Tested on x86_64-unknown-linux-gnu.
>>
>> [ I have a few more patches on top of these that together bring the total
>> number of valgrind warnings for the command "gdb gdb" down to ~30
>> but they are more controversial than these two, and if these aren't OK
>> then the rest definitely aren't OK. ]
>>
>> gdb/ChangeLog:
>>
>> * gdbarch.h (gdbarch_obstack_strdup): Declare.
>> * gdbarch.c (gdbarch_obstack_strdup): Define.
>> * gdbtypes.c (arch_type): Use it.
>
> Hi.
> A couple of comments.
>
> 1) gdbarch.[ch] are machine generated.
> IIUC, you have to edit gdbarch.sh and then run it to regenerate gdbarch.[ch].
Oops :/ I will fix this.
>
> 2) I would have done this slightly differently.
> If the obstack API doesn't provide a strdup functionality,
> I wouldn't insist on trying to add it there. But I would like
> to see it added to gdb in an application-independent way.
> (make it non-gdbarch specific). obstack_strdup sounds sufficiently
> useful and generic enough. If one wants to add a
> gdbarch_obstack_strdup wrapper on top of that, fine by me.
> IOW, add obstack_strdup to gdb_obstack.[ch].
And I will do this as well. Thanks for checking out these two patches.
@@ -449,6 +449,16 @@ gdbarch_obstack_zalloc (struct gdbarch *arch, long size)
return data;
}
+/* See gdbarch.h. */
+
+char *
+gdbarch_obstack_strdup (struct gdbarch *gdbarch, const char *string)
+{
+ char *obstring = gdbarch_obstack_zalloc (gdbarch, strlen (string) + 1);
+ strcpy (obstring, string);
+ return obstring;
+}
+
/* Free a gdbarch struct. This should never happen in normal
operation --- once you've created a gdbarch, you keep it around.
@@ -1614,6 +1614,11 @@ extern void *gdbarch_obstack_zalloc (struct gdbarch *gdbarch, long size);
#define GDBARCH_OBSTACK_CALLOC(GDBARCH, NR, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), (NR) * sizeof (TYPE)))
#define GDBARCH_OBSTACK_ZALLOC(GDBARCH, TYPE) ((TYPE *) gdbarch_obstack_zalloc ((GDBARCH), sizeof (TYPE)))
+/* Duplicate STRING, returning an equivalent string that's allocated on the
+ obstack associated with GDBARCH. The string is freed when the corresponding
+ architecture is also freed. */
+
+extern char *gdbarch_obstack_strdup (struct gdbarch *gdbarch, const char *string);
/* Helper function. Force an update of the current architecture.
@@ -4539,7 +4539,7 @@ arch_type (struct gdbarch *gdbarch,
TYPE_LENGTH (type) = length;
if (name)
- TYPE_NAME (type) = xstrdup (name);
+ TYPE_NAME (type) = gdbarch_obstack_strdup (gdbarch, name);
return type;
}