[ 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