Re: [committed] Don't call die_odr_state with unnecessarily defined cu arg

[Mark Wielaard <mark@klomp.org>]

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. It seems there is only one
call to die_odr_state that passes a non-NULL cu (the call in
checksum_die).

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).

Just wondering if when passing NULL in as cu the result isn't simply
always die->die_odr_state? (And if so, do we even need the function
call?)

Cheers,

Mark