* [PATCH] Clean up die_odr_state interface
2021-02-22 22:27 ` Mark Wielaard
@ 2021-02-23 10:00 ` Tom de Vries
2021-02-23 10:14 ` Mark Wielaard
0 siblings, 1 reply; 4+ messages in thread
From: Tom de Vries @ 2021-02-23 10:00 UTC (permalink / raw)
To: Mark Wielaard; +Cc: dwz, jakub
[-- Attachment #1: Type: text/plain, Size: 2518 bytes --]
[ was: Re: [committed] Don't call die_odr_state with unnecessarily
defined cu arg ]
On 2/22/21 11:27 PM, Mark Wielaard wrote:
> Hi Tom,
>
> On Mon, Feb 22, 2021 at 04:39:03PM +0100, Tom de Vries wrote:
>> Hi,
>>
>> When compiling dwz with this patch:
>> ...
>> die_odr_state (dw_cu_ref cu, dw_die_ref die)
>> {
>> if (die->die_odr_state != ODR_UNKNOWN)
>> - return die->die_odr_state;
>> + {
>> + assert (cu == NULL);
>> + return die->die_odr_state;
>> + }
>> ...
>> and running f.i. odr-struct.sh, we run into the abort.
>>
>> The recent commit 3312feb "Fix CK_BAD propagation for --odr" introduced this
>> code:
>> ...
>> if (die_odr_state (die_cu (die), die) != ODR_NONE)
>> die->u.p1.die_ref_hash = die->u.p1.die_hash;
>> ...
>> and there's no need to pass a CU argument, which makes the abort trigger.
>>
>> Fix this by passing a NULL CU instead.
>>
>> Committed to trunk.
>>
>> Thanks,
>> - Tom
>>
>> Don't call die_odr_state with unnecessarily defined cu arg
>>
>> 2021-02-22 Tom de Vries <tdevries@suse.de>
>>
>> * dwz.c (read_debug_info): Pass NULL CU to die_odr_state call.
>>
>> ---
>> dwz.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/dwz.c b/dwz.c
>> index 076f39c..86863ce 100644
>> --- a/dwz.c
>> +++ b/dwz.c
>> @@ -7253,7 +7253,7 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
>> {
>> if (die->die_ck_state != CK_KNOWN)
>> continue;
>> - if (die_odr_state (die_cu (die), die) != ODR_NONE)
>> + if (die_odr_state (NULL, die) != ODR_NONE)
>> die->u.p1.die_ref_hash = die->u.p1.die_hash;
>> else
>> die->die_ref_hash_computed = 0;
>
> I might be a little lost in the odr code.
Hi,
Yeah, I can imagine. Anyway, review much appreciated.
> It seems there is only one
> call to die_odr_state that passes a non-NULL cu (the call in
> checksum_die).
>
Ack, good observation.
> What does passing a NULL cu signify in this code? If cu is NULL it
> seems the code that calls set_die_odr_state at the end of
> die_ord_state is unsafe (because it might check cu).
>
Correct, so the NULL argument should only be used when set_die_odr_state
isn't called.
> Just wondering if when passing NULL in as cu the result isn't simply
> always die->die_odr_state?
It is.
> (And if so, do we even need the function
> call?)
I've left it in for now, for the sake of the assert. Instead, I've
dropped the cu parameter for the function.
WDYT?
Thanks,
- Tom
[-- Attachment #2: 0001-Clean-up-die_odr_state-interface.patch --]
[-- Type: text/x-patch, Size: 5871 bytes --]
Clean up die_odr_state interface
The die_odr_state function returns the odr state for a die, and if it hasn't
been calculated, it calculates it first using a call to set_die_odr_state:
...
static unsigned int UNUSED
die_odr_state (dw_cu_ref cu, dw_die_ref die)
{
if (die->die_odr_state != ODR_UNKNOWN)
return die->die_odr_state;
set_die_odr_state (cu, die);
return die->die_odr_state;
}
...
The call to set_die_odr_state needs a cu argument, and consequently
die_odr_state also has one. That means a call to die_odr_state needs a proper
CU argument if set_die_odr_state gets called, and for optimality a NULL CU
otherwise.
As it happens, there's only one place where the proper CU argument is
required, and it's easy to accidentally do:
...
die_odr_state (die_cu (die), die)
...
where a NULL CU would suffice.
Fix this by explicitly calling set_die_odr_state and dropping the cu parameter
from die_odr_state.
2021-02-23 Tom de Vries <tdevries@suse.de>
* dwz.c (set_die_odr_state): Assert die->die_odr_state == ODR_UNKNOWN.
(die_odr_state): Drop cu parameter. Assert
die->die_odr_state != ODR_UNKNOWN.
(checksum_die): Call set_die_odr_state. Update call to die_odr_state.
(read_debug_info, split_dups, reorder_dups, merged_singleton)
(partition_dups): Update call to die_odr_state.
---
dwz.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/dwz.c b/dwz.c
index 6c516cf..9fb11b9 100644
--- a/dwz.c
+++ b/dwz.c
@@ -3227,6 +3227,7 @@ set_die_odr_state (dw_cu_ref cu, dw_die_ref die)
bool name_p;
bool other_p;
+ assert (die->die_odr_state == ODR_UNKNOWN);
die->die_odr_state = ODR_NONE;
if (low_mem)
@@ -3310,12 +3311,9 @@ set_die_odr_state (dw_cu_ref cu, dw_die_ref die)
/* Return the initialized die_odr_state field for DIE with CU. */
static unsigned int UNUSED
-die_odr_state (dw_cu_ref cu, dw_die_ref die)
+die_odr_state (dw_die_ref die)
{
- if (die->die_odr_state != ODR_UNKNOWN)
- return die->die_odr_state;
-
- set_die_odr_state (cu, die);
+ assert (die->die_odr_state != ODR_UNKNOWN);
return die->die_odr_state;
}
@@ -3380,7 +3378,10 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
fprintf (stderr, "DIE %x, hash: %x, lang\n", die->die_offset,
die->u.p1.die_hash);
}
- only_hash_name_p = odr && die_odr_state (die_cu (die), die) != ODR_NONE;
+
+ if (odr && die->die_odr_state == ODR_UNKNOWN)
+ set_die_odr_state (die_cu (die), die);
+ only_hash_name_p = odr && die_odr_state (die) != ODR_NONE;
die_hash2 = 0;
if (only_hash_name_p)
die_hash2 = die->u.p1.die_hash;
@@ -7254,7 +7255,7 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
{
if (die->die_ck_state != CK_KNOWN)
continue;
- if (die_odr_state (NULL, die) != ODR_NONE)
+ if (die_odr_state (die) != ODR_NONE)
die->u.p1.die_ref_hash = die->u.p1.die_hash;
else
die->die_ref_hash_computed = 0;
@@ -7656,7 +7657,7 @@ split_dups (dw_die_ref die, struct obstack *vec)
for (i = 0; i < count; i++)
{
d = arr[i];
- if (die_odr_state (NULL, d) != ODR_DECL)
+ if (die_odr_state (d) != ODR_DECL)
continue;
if (!head)
head = d;
@@ -7675,14 +7676,14 @@ split_dups (dw_die_ref die, struct obstack *vec)
{
d = arr[i];
if (d->die_dup || d->die_nextdup
- || die_odr_state (NULL, d) == ODR_DECL)
+ || die_odr_state (d) == ODR_DECL)
continue;
tail = d;
for (j = i + 1; j < count; j++)
{
d2 = arr[j];
if (d2->die_dup || d2->die_nextdup
- || die_odr_state (NULL, d2) == ODR_DECL)
+ || die_odr_state (d2) == ODR_DECL)
continue;
die_eq (d, d2);
}
@@ -7696,7 +7697,7 @@ split_dups (dw_die_ref die, struct obstack *vec)
for (i = 0; i < count; i++)
{
d = arr[i];
- if (die_odr_state (NULL, d) == ODR_DECL
+ if (die_odr_state (d) == ODR_DECL
|| d->die_dup != NULL)
continue;
def = d;
@@ -7774,12 +7775,12 @@ reorder_dups (dw_die_ref die)
unsigned def_count = 0;
dw_die_ref d;
- if (die_odr_state (NULL, die) == ODR_NONE)
+ if (die_odr_state (die) == ODR_NONE)
return die;
for (d = die; d; d = d->die_nextdup)
{
- if (die_odr_state (NULL, d) == ODR_DECL)
+ if (die_odr_state (d) == ODR_DECL)
decl_count++;
else
def_count++;
@@ -7787,14 +7788,14 @@ reorder_dups (dw_die_ref die)
if (def_count == 0 || decl_count == 0)
return die;
- if (die_odr_state (NULL, die) != ODR_DECL)
+ if (die_odr_state (die) != ODR_DECL)
return die;
dw_die_ref def = NULL;
dw_die_ref prev = NULL;
for (d = die; d; prev = d, d = d->die_nextdup)
{
- if (die_odr_state (NULL, d) == ODR_DECL)
+ if (die_odr_state (d) == ODR_DECL)
continue;
def = d;
break;
@@ -8253,7 +8254,7 @@ merged_singleton (dw_die_ref die)
size_t decl_cnt = 0;
for (d = die; d; d = d->die_nextdup)
- switch (die_odr_state (NULL, d))
+ switch (die_odr_state (d))
{
case ODR_DEF:
if (res)
@@ -8313,7 +8314,7 @@ partition_dups (void)
for (i = 0; i < vec_size; i++)
{
dw_die_ref die = arr[i];
- if (die_odr_state (NULL, die) == ODR_NONE)
+ if (die_odr_state (die) == ODR_NONE)
continue;
die = split_dups (die, &ob2);
assert (die != NULL);
@@ -8340,7 +8341,7 @@ partition_dups (void)
&& die->die_nextdup == NULL)
mark_singletons (die_cu (die), die, die, &ob2);
else if (odr_mode != ODR_BASIC
- && die_odr_state (NULL, die) != ODR_NONE)
+ && die_odr_state (die) != ODR_NONE)
{
dw_die_ref s = merged_singleton (die);
if (s)
^ permalink raw reply [flat|nested] 4+ messages in thread