Em Wed, Apr 03, 2019 at 12:01:45AM -0700, andrii.nakryiko@xxxxxxxxx escreveu:
> From: Andrii Nakryiko <andriin@xxxxxx>
>
> pahole --reorganize heavily depends on member's bit_hole and hole fields
> to denote bit/byte holes *after* member. Previous commit
> "dwarves: use bit sizes and bit/byte hole info in __class__fprintf"
> changed its meaning to bit/byte hole *before* member to accomodate
> possible bit/byte holes at the beginning of a struct. This change broke
> reorganization algorithm, though, which is quite involved and isn't
> trivially modifiable to accomodate new semantics.
My plan was to rewrite the reorganization algorithms, starting with the
simplest one, tagging v1.13 not to get in the way of using pahole to
build the kernel, then for v1.14 I'd start reimplementing bitfield
demotion, bitfield merging and other reorganize methods to compress a
struct.
But even that would take a while, so I'll try your patches and if all is
well, try to tag v1.13, then we can revisit all this if needed.
Thanks for working on this!
> This patch reverts the meaning of bit_hole and hole, but also introduces
> per class pre_bit_hole/pre_hole to record initial bit/byte hole of
> a struct. This allows to fix reorg code more easily and still handle
> initial holes cases, if at the expense of being not as elegant.
For a later version we can try to have both and after writing new reorg
algorithms to switch to using the most elegant one. Its just that there
are many details, so doing it step by step and comparing the results, as
we did with btfdiff I think is the most sensible path, one that will
help us validate it and find bugs in both sides, fixing it and having
something more solid overall.
- Arnaldo
> Signed-off-by: Andrii Nakryiko <andriin@xxxxxx>
> ---
> dwarves.c | 9 ++++--
> dwarves.h | 2 ++
> dwarves_fprintf.c | 72 +++++++++++++++++++++++++++++++----------------
> 3 files changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/dwarves.c b/dwarves.c
> index 619dcb3..efa58a9 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -1260,8 +1260,13 @@ void class__find_holes(struct class *class)
> cur_bitfield_end = bit_end;
> }
>
> - pos->hole = byte_holes;
> - pos->bit_hole = bit_holes;
> + if (last) {
> + last->hole = byte_holes;
> + last->bit_hole = bit_holes;
> + } else {
> + class->pre_hole = byte_holes;
> + class->pre_bit_hole = bit_holes;
> + }
> if (bit_holes)
> class->nr_bit_holes++;
> if (byte_holes)
> diff --git a/dwarves.h b/dwarves.h
> index e39871f..46083da 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -1026,7 +1026,9 @@ struct class {
> uint16_t nr_vtable_entries;
> uint8_t nr_holes;
> uint8_t nr_bit_holes;
> + uint16_t pre_hole;
> uint16_t padding;
> + uint8_t pre_bit_hole;
> uint8_t bit_padding;
> bool holes_searched;
> void *priv;
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index faae3ba..ef53e2b 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -1292,6 +1292,30 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
>
> printed += fprintf(fp, " {\n");
>
> + if (class->pre_bit_hole > 0 && !cconf.suppress_comments) {
> + if (!newline++) {
> + fputc('\n', fp);
> + ++printed;
> + }
> + printed += fprintf(fp, "%.*s/* XXX %d bit%s hole, "
> + "try to pack */\n", cconf.indent, tabs,
> + class->pre_bit_hole,
> + class->pre_bit_hole != 1 ? "s" : "");
> + sum_bit_holes += class->pre_bit_hole;
> + }
> +
> + if (class->pre_hole > 0 && !cconf.suppress_comments) {
> + if (!newline++) {
> + fputc('\n', fp);
> + ++printed;
> + }
> + printed += fprintf(fp, "%.*s/* XXX %d byte%s hole, "
> + "try to pack */\n",
> + cconf.indent, tabs, class->pre_hole,
> + class->pre_hole != 1 ? "s" : "");
> + sum_holes += class->pre_hole;
> + }
> +
> type__for_each_tag(type, tag_pos) {
> struct tag *type;
> const char *accessibility = tag__accessibility(tag_pos);
> @@ -1361,30 +1385,6 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
> }
> }
>
> - if (pos->bit_hole != 0 && !cconf.suppress_comments) {
> - if (!newline++) {
> - fputc('\n', fp);
> - ++printed;
> - }
> - printed += fprintf(fp, "%.*s/* XXX %d bit%s hole, "
> - "try to pack */\n", cconf.indent, tabs,
> - pos->bit_hole,
> - pos->bit_hole != 1 ? "s" : "");
> - sum_bit_holes += pos->bit_hole;
> - }
> -
> - if (pos->hole > 0 && !cconf.suppress_comments) {
> - if (!newline++) {
> - fputc('\n', fp);
> - ++printed;
> - }
> - printed += fprintf(fp, "%.*s/* XXX %d byte%s hole, "
> - "try to pack */\n",
> - cconf.indent, tabs, pos->hole,
> - pos->hole != 1 ? "s" : "");
> - sum_holes += pos->hole;
> - }
> -
> if (newline) {
> fputc('\n', fp);
> newline = 0;
> @@ -1428,6 +1428,30 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
> }
> }
>
> + if (pos->bit_hole != 0 && !cconf.suppress_comments) {
> + if (!newline++) {
> + fputc('\n', fp);
> + ++printed;
> + }
> + printed += fprintf(fp, "\n%.*s/* XXX %d bit%s hole, "
> + "try to pack */", cconf.indent, tabs,
> + pos->bit_hole,
> + pos->bit_hole != 1 ? "s" : "");
> + sum_bit_holes += pos->bit_hole;
> + }
> +
> + if (pos->hole > 0 && !cconf.suppress_comments) {
> + if (!newline++) {
> + fputc('\n', fp);
> + ++printed;
> + }
> + printed += fprintf(fp, "\n%.*s/* XXX %d byte%s hole, "
> + "try to pack */",
> + cconf.indent, tabs, pos->hole,
> + pos->hole != 1 ? "s" : "");
> + sum_holes += pos->hole;
> + }
> +
> fputc('\n', fp);
> ++printed;
>
> --
> 2.17.1
--
- Arnaldo
