Re: [RFA/DWARF] Set enum type "flag_enum" and "unsigned" flags at type creation.

[Joel Brobecker <brobecker@adacore.com>]

Hi Mark,

> > This looks suspicious. I think the explicit sign-extension is in general
> > wrong. It seems to assume that the DWARF producer encoded the
> > DW_AT_upper_bound wrongly (not as a signed value, but as an unsigned
> > value that needs to be sign-extended). Something like that might happen
> > in theory if the producer used DW_FORM_data[1248] because DWARF doesn't
> > define how to encode signed values in that case. But in practice this
> > seems to have been settled by interpreting these values as zero-extended
> > values (not sign-extended) and by the producer using either
> > DW_FORM_sdata or DW_FORM_udata to remove any ambiguity (like in your
> > testcase).

Thanks again for looking at the patch, and for your comments.

Interestingly, I had thought at the time that the only way to express
signedness of the underlying value was by using a base type as a subtype
(IIRC) which, of course, if a lot of data for one small attribute.
The use of the form seems like a much more efficient way to achieve
that goal. But the above explains why I interpreted the current code
as a way to work around the fact that compilers might not be emitting
the required subtype for enums with negative values. Not sure how
correct or not that interpretation was...

> > Does anything break if you just remove the sign-extension part?
> > If not, then you don't have to go through the whole
> > update_enumeration_type_from_children. Or do you need that for anything
> > else?
> 
> So, this patch doesn't show any regressions in the testsuite:

I will verify your fix against my testcase as well, but I like the idea
of removing code that should normally not be there.

But to answer your question above (do I also need my patch?), I think
the need might become less obvious once your patch is in. But I also
think it would probably be cleaner to have a complete type right from
the get-go, especially since I don't think the patch actually
complexifies the code (maybe even the opposite). That being said,
I'm not strongly attached to it, as long as GDB does TRT :-).

Thanks again for your feedback!

> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 54c538a..0b5de99 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -14303,7 +14303,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
>    LONGEST low, high;
>    int low_default_is_valid;
>    const char *name;
> -  LONGEST negative_mask;
>  
>    orig_base_type = die_type (die, cu);
>    /* If ORIG_BASE_TYPE is a typedef, it will not be TYPE_UNSIGNED,
> @@ -14433,13 +14432,6 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
>  	}
>      }
>  
> -  negative_mask =
> -    (LONGEST) -1 << (TYPE_LENGTH (base_type) * TARGET_CHAR_BIT - 1);
> -  if (!TYPE_UNSIGNED (base_type) && (low & negative_mask))
> -    low |= negative_mask;
> -  if (!TYPE_UNSIGNED (base_type) && (high & negative_mask))
> -    high |= negative_mask;
> -
>    range_type = create_range_type (NULL, orig_base_type, low, high);
>  
>    /* Mark arrays with dynamic length at least as an array of unspecified
> 
> 
> So, my hope is that sign extension hack really isn't needed.
> Of course it could be that there is some case where it was really needed
> and there just isn't a test case for it. Does anybody know/remember?
> 
> Thanks,
> 
> Mark

-- 
Joel