On 04/09/2024 17:41, Will Hawkins wrote:
> In certain cases, pointer modifiers were attached to the wrong part of a type.
>
Nit but I'd expand the commit message here to describe a bit more; even
including the cover letter info here would be ideal for the commit log
as it helps hugely in understanding the issue.
> Signed-off-by: Will Hawkins <hawkinsw@xxxxxx>
Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
small things about comments etc below tho..
> ---
> dwarves_fprintf.c | 57 ++++++++++++++---------------------------------
> 1 file changed, 17 insertions(+), 40 deletions(-)
>
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 54d2fc2..6f1f997 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),
> @@ -617,20 +607,30 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> 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.
again a tiny thing but in general /* */ style comments are preferred for
multi-line comments; I know there's exceptions (even in this file) but
it's the most common approach used across the codebase I think.
also a newline between declaration + comments is no harm too.
I think it might be no harm to provide a simple example in a comment
that relates tag spec + C representation to DWARF/BTF representation as
it's a tricky area. So something along the lines of:
"Reminder: "const int *foo" means that the target of the pointer is
const, while "const int * volatile foo" means that the target of the
pointer is const ("const int *") _and_ the pointer itself (volatile foo)
is volatile.
This is confusing because the later volatile refers to the pointer,
while the earlier const refers to the target type.
The DWARF or BTF representation will be of the more natural form
volatile -> ptr -> const -> int
This reads more naturally as a volatile pointer to a constant int. To
display it 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."
Doesn't have to be that wordy of course (and I hope I got the details
right!) but I think this is a tricky area, so might be worth adding a
comment here explaining the rationale.
The change itself looks good to me and I _think_ recursion takes care of
multipe nested oddball cases too. Seems to clean up the logic too
removing the goto print_default. Nice work!
> + 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 +833,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 +868,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 +876,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;
