Re: [PATCH v2 3/5] symbols: arrange to know where functions end

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2025-04-02 09:58, Jan Beulich wrote:
> When determining the symbol for a given address (e.g. for the %pS
> logging format specifier), so far the size of a symbol (function) was
> assumed to be everything until the next symbol. There may be gaps
> though, which would better be recognizable in output (often suggesting
> something odd is going on).
>
> Insert "fake" end symbols in the address table, accompanied by zero-
> length type/name entries (to keep lookup reasonably close to how it
> was).
>
> Note however that this, with present GNU binutils, won't work for
> xen.efi: The linker loses function sizes (they're not part of a normal
> symbol table entry), and hence nm has no way of reporting them.
>
> The address table growth is quite significant on x86 release builds (due
> to functions being aligned to 16-byte boundaries), though: Its size
> almost doubles.
>
> Requested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Note: Style-wise this is a horrible mix. I'm trying to match styles with
>        what's used in the respective functions.
>
> Older GNU ld retains section symbols, which nm then also lists. Should
> we perhaps strip those as we read in nm's output? They don't provide any
> useful extra information, as our linker scripts add section start
> symbols anyway. (For the purposes here, luckily such section symbols are
> at least emitted without size.)
>
> Even for section start symbols there is the question of whether they
> really need retaining (except perhaps when producing a map file). The
> main question here likely is whether livepatch may have a need to look
> them up by name. (Section end symbols may actually be slightly more
> useful to keep, but that may also want considering more closely.)
> ---
> --- a/xen/tools/symbols.c
> +++ b/xen/tools/symbols.c
> @@ -318,24 +334,42 @@ static void write_src(void)
>         printf("#else\n");
>         output_label("symbols_offsets");
>         printf("#endif\n");
> -       for (i = 0; i < table_cnt; i++) {
> +       for (i = 0, ends = 0; i < table_cnt; i++) {
>                 printf("\tPTR\t%#llx - SYMBOLS_ORIGIN\n", table[i].addr);
> +
> +               table[i].addr_idx = i + ends;
> +
> +               if (!want_symbol_end(i)) {
> +                       /* If there's another symbol at the same address,
> +                        * propagate this symbol's size if the next one has
> +                        * no size, or if the next one's size is larger. */
Why do we want to shrink the next symbol's size?

The code looks good - I just don't understand this condition.

Thanks,
Jason

> +                       if (table[i].size &&
> +                           i + 1 < table_cnt &&
> +                           table[i + 1].addr == table[i].addr &&
> +                           (!table[i + 1].size ||
> +                            table[i + 1].size > table[i].size))
> +                               table[i + 1].size = table[i].size;
> +                       continue;
> +               }
> +
> +               ++ends;
> +               printf("\tPTR\t%#llx - SYMBOLS_ORIGIN\n",
> +                      table[i].addr + table[i].size);
>         }
>         printf("\n");
>   
>   	output_label("symbols_num_addrs");
> -       printf("\t.long\t%d\n", table_cnt);
> +       printf("\t.long\t%d\n", table_cnt + ends);
>         printf("\n");
>   
>   	/* table of offset markers, that give the offset in the compressed stream
>          * every 256 symbols */
> -       markers = (unsigned int *) malloc(sizeof(unsigned int) * ((table_cnt + 
> 255) / 256));
> +       markers = malloc(sizeof(*markers) * ((table_cnt + ends + 255) >> 8));
>   
>   	output_label("symbols_names");
> -       off = 0;
> -       for (i = 0; i < table_cnt; i++) {
> -               if ((i & 0xFF) == 0)
> -                       markers[i >> 8] = off;
> +       for (i = 0, off = 0, ends = 0; i < table_cnt; i++) {
> +               if (((i + ends) & 0xFF) == 0)
> +                       markers[(i + ends) >> 8] = off;
>   
>   		printf("\t.byte 0x%02x", table[i].len);
>                 for (k = 0; k < table[i].len; k++)