* [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
@ 2012-04-07 19:39 Doug Evans
2012-04-07 20:40 ` Michael Eager
0 siblings, 1 reply; 6+ messages in thread
From: Doug Evans @ 2012-04-07 19:39 UTC (permalink / raw)
To: gdb-patches
Hi.
There's no caller where cu->per_cu == NULL,
and I'd rather have the assert.
Regression tested on amd64-linux.
2012-04-07 Doug Evans <dje@google.com>
* dwarf2read.c (load_partial_dies): Change condition to assert.
Index: dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.627
diff -u -p -r1.627 dwarf2read.c
--- dwarf2read.c 19 Mar 2012 19:59:19 -0000 1.627
+++ dwarf2read.c 7 Apr 2012 19:33:59 -0000
@@ -9522,7 +9522,8 @@ load_partial_dies (bfd *abfd, gdb_byte *
parent_die = NULL;
last_die = NULL;
- if (cu->per_cu && cu->per_cu->load_all_dies)
+ gdb_assert (cu->per_cu != NULL);
+ if (cu->per_cu->load_all_dies)
load_all = 1;
cu->partial_dies
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
2012-04-07 19:39 [commit] dwarf2read.c (load_partial_dies): Change condition to assert Doug Evans
@ 2012-04-07 20:40 ` Michael Eager
2012-04-07 20:49 ` Doug Evans
0 siblings, 1 reply; 6+ messages in thread
From: Michael Eager @ 2012-04-07 20:40 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On 04/07/2012 12:39 PM, Doug Evans wrote:
> Hi.
>
> There's no caller where cu->per_cu == NULL,
> and I'd rather have the assert.
Per_cu data is flushed asynchronously in free_heap_comp_unit()
and free_stack_comp_unit(). Are you sure that this can't happen?
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
2012-04-07 20:40 ` Michael Eager
@ 2012-04-07 20:49 ` Doug Evans
2012-04-07 21:46 ` Michael Eager
0 siblings, 1 reply; 6+ messages in thread
From: Doug Evans @ 2012-04-07 20:49 UTC (permalink / raw)
To: Michael Eager; +Cc: gdb-patches
On Sat, Apr 7, 2012 at 1:40 PM, Michael Eager <eager@eagerm.com> wrote:
> On 04/07/2012 12:39 PM, Doug Evans wrote:
>>
>> Hi.
>>
>> There's no caller where cu->per_cu == NULL,
>> and I'd rather have the assert.
>
>
> Per_cu data is flushed asynchronously in free_heap_comp_unit()
> and free_stack_comp_unit(). Are you sure that this can't happen?
Define "asynchronously".
My reading of all of the callers says it can't happen (modulo bugs of course).
I could have missed something of course (in which case let's get
something added to the testsuite to exercise the appropriate code
path).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
2012-04-07 20:49 ` Doug Evans
@ 2012-04-07 21:46 ` Michael Eager
2012-04-07 22:14 ` Doug Evans
0 siblings, 1 reply; 6+ messages in thread
From: Michael Eager @ 2012-04-07 21:46 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On 04/07/2012 01:49 PM, Doug Evans wrote:
> On Sat, Apr 7, 2012 at 1:40 PM, Michael Eager<eager@eagerm.com> wrote:
>> On 04/07/2012 12:39 PM, Doug Evans wrote:
>>>
>>> Hi.
>>>
>>> There's no caller where cu->per_cu == NULL,
>>> and I'd rather have the assert.
>>
>> Per_cu data is flushed asynchronously in free_heap_comp_unit()
>> and free_stack_comp_unit(). Are you sure that this can't happen?
>
> Define "asynchronously".
age_cached_comp_units() is called to age the CU cache and free up
cached CUs which were not recently used. This happens while reading
symbols for a different CU.
> My reading of all of the callers says it can't happen (modulo bugs of course).
> I could have missed something of course (in which case let's get
> something added to the testsuite to exercise the appropriate code
> path).
My concern is that unless the per_cu data was loaded by one of the
immediate callers, it may have been flushed.
I'm not sure I can create a reasonable test case for this problem.
I ran into this with a large program with many CUs printing a struct
which took several pages to print. The per_cu data disappeared in
the middle of printing the struct.
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
2012-04-07 21:46 ` Michael Eager
@ 2012-04-07 22:14 ` Doug Evans
2012-04-07 22:27 ` Michael Eager
0 siblings, 1 reply; 6+ messages in thread
From: Doug Evans @ 2012-04-07 22:14 UTC (permalink / raw)
To: Michael Eager; +Cc: gdb-patches
On Sat, Apr 7, 2012 at 2:46 PM, Michael Eager <eager@eagerm.com> wrote:
> age_cached_comp_units() is called to age the CU cache and free up
> cached CUs which were not recently used. This happens while reading
> symbols for a different CU.
Right, but such calls won't happen between the time cu->per_cu is set
and load_partial_dies is called (unless I've missed something of
course).
>> My reading of all of the callers says it can't happen (modulo bugs of
>> course).
>> I could have missed something of course (in which case let's get
>> something added to the testsuite to exercise the appropriate code
>> path).
>
>
> My concern is that unless the per_cu data was loaded by one of the
> immediate callers, it may have been flushed.
I welcome someone reviewing the callers and showing what I missed.
I know of no case where a struct dwarf2_cu exists without its
corresponding struct dwarf2_per_cu_data.
> I'm not sure I can create a reasonable test case for this problem.
> I ran into this with a large program with many CUs printing a struct
> which took several pages to print. The per_cu data disappeared in
> the middle of printing the struct.
I think we need to be careful with terminology here (gdb sometimes
doesn't make this easy though).
There's per_cu and there's per_cu->cu.
per_cu (i.e. struct dwarf2_per_cu_data) does not get flushed or aged.
per_cu->cu (i.e. struct dwarf2_cu) does.
In your case did a struct dwarf2_per_cu_data disappear, or was it per_cu->cu?
I have no doubt there are bugs in this area, btw.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [commit] dwarf2read.c (load_partial_dies): Change condition to assert.
2012-04-07 22:14 ` Doug Evans
@ 2012-04-07 22:27 ` Michael Eager
0 siblings, 0 replies; 6+ messages in thread
From: Michael Eager @ 2012-04-07 22:27 UTC (permalink / raw)
To: Doug Evans; +Cc: gdb-patches
On 04/07/2012 03:13 PM, Doug Evans wrote:
> I think we need to be careful with terminology here (gdb sometimes
> doesn't make this easy though).
> There's per_cu and there's per_cu->cu.
> per_cu (i.e. struct dwarf2_per_cu_data) does not get flushed or aged.
> per_cu->cu (i.e. struct dwarf2_cu) does.
>
> In your case did a struct dwarf2_per_cu_data disappear, or was it per_cu->cu?
Probably the per_cu->cu. The terminology and the many uses of
the same or similar names make remembering the details difficult.
> I have no doubt there are bugs in this area, btw.
Yup.
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-07 22:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-07 19:39 [commit] dwarf2read.c (load_partial_dies): Change condition to assert Doug Evans
2012-04-07 20:40 ` Michael Eager
2012-04-07 20:49 ` Doug Evans
2012-04-07 21:46 ` Michael Eager
2012-04-07 22:14 ` Doug Evans
2012-04-07 22:27 ` Michael Eager
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox