On Wed, Sep 25, 2024 at 6:07 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> On 19/09/2024 04:30, Will Hawkins wrote:
> > In certain cases, type qualifiers were attached to the wrong part
> > of a type.
> >
> > In C-like languages, a basic variable declaration can be augmented
> > with certain _qualifiers_. Qualifiers are used in declarations by the
> > programmer to specify additional properties about the type being
> > declared. When it comes to applying qualifiers to pointer types,
> > placement can sometimes be tricky.
> >
> > Because of the hierarchical nature of type encoding in both BTF and
> > DWARF, the representation of such confusing types is more natural.
> >
> > For example
> > volatile -> ptr -> const -> int
> > is the "chained" list of types that will exist (with a few minor
> > differences between the formats) for BTF and DWARF to encode a
> > volatile pointer to a constant int.
> >
> > To display types like these correctly, the modifier that applies
> > to the pointer must be a suffix modifier printed after the pointer,
> > and the modifier that applies to the pointer target must be printed
> > before the pointer.
> >
> > This patch teaches the dwarves family of programs to print qualified
> > pointer types correctly.
> >
> > Signed-off-by: Will Hawkins <hawkinsw@xxxxxx>
>
> Looks great, thanks for adding these valuable descriptions!
>
> Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
>
> Future work for us as a community in general is to improve automated
> testing ; in the case of functionality like this, we could perhaps look
> at adding some simple programs that exercise complex type relationships
> and verify that everything is printed as expected. If you wanted to add
> a test in this area, your example from the v1 cover letter would be a
> great addition. We've started populating the tests/ directory, but more
> contributions in that area would be really valuable. Thanks again!
Thank you (again!) for being so welcoming! I saw that you were
starting to populate the test directory and I *will* start adding
tests.
Again, thank you for everything!
Will
>
> Alan
>
> > ---
> >
> > Notes:
> > v1 -> v2:
> > Add additional comments inline.
> > Properly format comments using /**/.
> > Be more verbose in the commit message.
> >
> > dwarves_fprintf.c | 90 ++++++++++++++++++++++++++---------------------
> > 1 file changed, 50 insertions(+), 40 deletions(-)
> >
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index 54d2fc2..2c53638 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -542,16 +542,6 @@ static const char *tag__ptr_name(const struct tag *tag, const struct cu *cu,
> > char tmpbf[1024];
> > const char *const_pointer = "";
> >
> > - if (tag__is_const(type)) {
> > - struct tag *next_type = cu__type(cu, type->type);
> > -
> > - if (next_type && tag__is_pointer(next_type)) {
> > - if (!(conf && conf->skip_emitting_modifier))
> > - const_pointer = "const ";
> > - type = next_type;
> > - }
> > - }
> > -
> > snprintf(bf, len, "%s %s%s",
> > __tag__name(type, cu,
> > tmpbf, sizeof(tmpbf), conf),
> > @@ -612,25 +602,68 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> > case DW_TAG_restrict_type:
> > case DW_TAG_atomic_type:
> > case DW_TAG_unspecified_type:
> > + /*
> > + * In C-like languages, a basic variable declaration can be augmented
> > + * with certain _qualifiers_. In C++, the standard refers to them as
> > + * cv-qualifiers (decl.type.cv). In C, the standard refers to them as
> > + * type qualifiers (6.7.3). Qualifiers are used in declarations by the
> > + * programmer to specify additional properties about the type being
> > + * declared. When it comes to applying qualifiers to pointer types,
> > + * placement can sometimes be tricky.
> > + * Example 1: "const int * foo": The target of the pointer is const.
> > + * Example 2: "const int * volatile foo" means that the target of the
> > + * pointer is const * ("const int *") _and_ the pointer itself (volatile
> > + * foo) is volatile.
> > + * Example 3: "volatile int * volatile foo" means that both the target
> > + * of foo _and_ the pointer itself are volatile.
> > + *
> > + * Note: cdecl(1) is a fun command-line tool for helping decode the
> > + * meaning of these complicated type signatures.
> > + *
> > + * Because of the hierarchical nature of type encoding in both BTF and
> > + * DWARF, the representation of such confusing types is more natural.
> > + * For example
> > + *
> > + * volatile -> ptr -> const -> int
> > + *
> > + * is the "chained" list of types that will exist (with a few minor
> > + * differences between the formats) for BTF and DWARF to encode a
> > + * volatile pointer to a constant int. To display types like these
> > + * correctly, the modifier that applies to the pointer must be a
> > + * suffix modifier printed after the pointer, and the modifier that
> > + * applies to the pointer target must be printed before the pointer.
> > + */
> > type = cu__type(cu, tag->type);
> > if (type == NULL && tag->type != 0)
> > tag__id_not_found_snprintf(bf, len, tag->type);
> > else if (!tag__has_type_loop(tag, type, bf, len, NULL)) {
> > char tmpbf[128];
> > - const char *prefix = "", *suffix = "",
> > + const char *prefix_modifier = NULL, *suffix_modifier = NULL,
> > *type_str = __tag__name(type, cu, tmpbf,
> > sizeof(tmpbf),
> > pconf);
> > if (!pconf->skip_emitting_modifier) {
> > + const char **modifier = &prefix_modifier;
> > +
> > + /* If the modifier is addressing a pointer, then we will need
> > + * put it after the type that is being printed.
> > + */
> > + if (tag__is_pointer(type))
> > + modifier = &suffix_modifier;
> > +
> > switch (tag->tag) {
> > - case DW_TAG_volatile_type: prefix = "volatile "; break;
> > - case DW_TAG_const_type: prefix = "const "; break;
> > - case DW_TAG_restrict_type: suffix = " restrict"; break;
> > - case DW_TAG_atomic_type: prefix = "_Atomic "; break;
> > + case DW_TAG_volatile_type: *modifier = "volatile"; break;
> > + case DW_TAG_const_type: *modifier = "const"; break;
> > + case DW_TAG_restrict_type: suffix_modifier = " restrict"; break;
> > + case DW_TAG_atomic_type: *modifier = "_Atomic"; break;
> > }
> > }
> > - snprintf(bf, len, "%s%s%s%s", prefix, type_str, suffix,
> > - pconf->no_parm_names ? "" : " ");
> > + snprintf(bf, len, "%s%s%s%s%s%s", prefix_modifier ? prefix_modifier : "",
> > + prefix_modifier ? " " : "",
> > + type_str,
> > + suffix_modifier ? " " : "",
> > + suffix_modifier ? suffix_modifier : "",
> > + pconf->no_parm_names ? "" : " ");
> > }
> > break;
> > case DW_TAG_array_type:
> > @@ -833,8 +866,6 @@ inner_struct:
> > tconf.suppress_offset_comment = suppress_offset_comment;
> > }
> >
> > - const char *modifier;
> > -
> > next_type:
> > switch (type->tag) {
> > case DW_TAG_pointer_type: {
> > @@ -870,7 +901,6 @@ next_type:
> > /* Fall Thru */
> > }
> > default:
> > -print_default:
> > printed += fprintf(fp, "%-*s %s", tconf.type_spacing,
> > tag__name(type, cu, tbf, sizeof(tbf), &tconf),
> > name);
> > @@ -879,26 +909,6 @@ print_default:
> > printed += ftype__fprintf(tag__ftype(type), cu, name, 0, 0,
> > tconf.type_spacing, true, &tconf, fp);
> > break;
> > - case DW_TAG_atomic_type:
> > - modifier = "_Atomic";
> > - goto print_modifier;
> > - case DW_TAG_const_type:
> > - modifier = "const";
> > -print_modifier: {
> > - if (!conf->skip_emitting_modifier) {
> > - size_t modifier_printed = fprintf(fp, "%s ", modifier);
> > - tconf.type_spacing -= modifier_printed;
> > - printed += modifier_printed;
> > - }
> > -
> > - struct tag *ttype = cu__type(cu, type->type);
> > - if (ttype) {
> > - type = ttype;
> > - goto next_type;
> > - }
> > - }
> > - goto print_default;
> > -
> > case DW_TAG_array_type:
> > printed += array_type__fprintf(type, cu, name, &tconf, fp);
> > break;
>
