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!
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;
