[PATCH pahole] loaders: strip away volatile/const/restrict when fixing bitfields

[PATCH pahole] loaders: strip away volatile/const/restrict when fixing bitfields

[Andrii Nakryiko]

btf_loader and ctf_loader didn't remove const/volatile/restrict, so
bitfields using modifiers were not adjusted properly.

This patch abstracts logic of stripping aways typedefs and access
modifiers into tag__strip_typedefs_and_modifiers, which handles any
interleaving of typedefs and modifiers. dwarf_loader was adapter to
reuse this function as well, instead of custom goto loop.

REPRO:
$ cat vc_map.c
typedef unsigned int u32;
typedef volatile u32 vu32;
typedef vu32 vu32_t;

typedef struct vc_map {
        volatile unsigned int tx: 1;
        vu32_t rx: 1;
        void *x1, *x2;
} vc_map;

int main() {
        struct vc_map s;
        return 0;
}

BEFORE:
$ ~/pahole/build/pahole -F btf vc_map
struct vc_map {
        volatile unsigned int      tx:1;                 /*     0: 0  4 */
        vu32_t                     rx:1;                 /*     0: 0  4 */

        /* XXX 30 bits hole, try to pack */
        /* XXX 4 bytes hole, try to pack */

        void *                     x1;                   /*     8     8 */
        void *                     x2;                   /*    16     8 */

        /* size: 24, cachelines: 1, members: 4 */
        /* sum members: 20, holes: 1, sum holes: 4 */
        /* bit holes: 1, sum bit holes: 30 bits */
        /* last cacheline: 24 bytes */
};

AFTER:
$ ~/pahole/build/pahole -F btf vc_map
struct vc_map {
        volatile unsigned int      tx:1;                 /*     0:31  4 */
        vu32_t                     rx:1;                 /*     0:30  4 */

        /* XXX 30 bits hole, try to pack */
        /* XXX 4 bytes hole, try to pack */

        void *                     x1;                   /*     8     8 */
        void *                     x2;                   /*    16     8 */

        /* size: 24, cachelines: 1, members: 4 */
        /* sum members: 20, holes: 1, sum holes: 4 */
        /* bit holes: 1, sum bit holes: 30 bits */
        /* last cacheline: 24 bytes */
};

Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
 btf_loader.c   |  2 +-
 ctf_loader.c   |  2 +-
 dwarf_loader.c |  7 +------
 dwarves.c      | 10 ++++++++++
 dwarves.h      | 13 +++++++++++++
 5 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 9a0714a..e04175e 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -457,7 +457,7 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu, struct btf
 	struct type *tag_type = tag__type(tag);
 
 	type__for_each_data_member(tag_type, pos) {
-		struct tag *type = tag__follow_typedef(&pos->tag, cu);
+		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
 
 		if (type == NULL) /* FIXME: C++ BTF... */
 			continue;
diff --git a/ctf_loader.c b/ctf_loader.c
index 1631ea7..68479e2 100644
--- a/ctf_loader.c
+++ b/ctf_loader.c
@@ -613,7 +613,7 @@ static int class__fixup_ctf_bitfields(struct tag *tag, struct cu *cu)
 	struct type *tag_type = tag__type(tag);
 
 	type__for_each_data_member(tag_type, pos) {
-		struct tag *type = tag__follow_typedef(&pos->tag, cu);
+		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
 
 		if (type == NULL) /* FIXME: C++ CTF... */
 			continue;
diff --git a/dwarf_loader.c b/dwarf_loader.c
index c2abdad..28d1393 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -2123,12 +2123,7 @@ static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,
 	}
 
 	if (member->bitfield_size != 0) {
-		struct tag *type = tag__follow_typedef(&member->tag, cu);
-check_volatile:
-		if (tag__is_volatile(type) || tag__is_const(type)) {
-			type = tag__follow_typedef(type, cu);
-			goto check_volatile;
-		}
+		struct tag *type = tag__strip_typedefs_and_modifiers(&member->tag, cu);
 
 		uint16_t type_bit_size;
 		size_t integral_bit_size;
diff --git a/dwarves.c b/dwarves.c
index 4c6a12c..0b95d37 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -128,6 +128,16 @@ struct tag *tag__follow_typedef(const struct tag *tag, const struct cu *cu)
 	return type;
 }
 
+struct tag *tag__strip_typedefs_and_modifiers(const struct tag *tag, const struct cu *cu)
+{
+	struct tag *type = cu__type(cu, tag->type);
+
+	while (type != NULL && (tag__is_typedef(type) || tag__is_modifier(type)))
+		type = cu__type(cu, type->type);
+
+	return type;
+}
+
 size_t __tag__id_not_found_fprintf(FILE *fp, type_id_t id,
 				   const char *fn, int line)
 {
diff --git a/dwarves.h b/dwarves.h
index 67ec014..f65dbe2 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -411,6 +411,18 @@ static inline bool tag__is_volatile(const struct tag *tag)
 	return tag->tag == DW_TAG_volatile_type;
 }
 
+static inline bool tag__is_restrict(const struct tag *tag)
+{
+	return tag->tag == DW_TAG_restrict_type;
+}
+
+static inline int tag__is_modifier(const struct tag *tag)
+{
+	return tag__is_const(tag) ||
+	       tag__is_volatile(tag) ||
+	       tag__is_restrict(tag);
+}
+
 static inline bool tag__has_namespace(const struct tag *tag)
 {
 	return tag__is_struct(tag) ||
@@ -498,6 +510,7 @@ void tag__not_found_die(const char *file, int line, const char *func);
 size_t tag__size(const struct tag *tag, const struct cu *cu);
 size_t tag__nr_cachelines(const struct tag *tag, const struct cu *cu);
 struct tag *tag__follow_typedef(const struct tag *tag, const struct cu *cu);
+struct tag *tag__strip_typedefs_and_modifiers(const struct tag *tag, const struct cu *cu);
 
 size_t __tag__id_not_found_fprintf(FILE *fp, type_id_t id,
 				   const char *fn, int line);
-- 
2.17.1

Re: [PATCH pahole] loaders: strip away volatile/const/restrict when fixing bitfields

[Arnaldo Carvalho de Melo]

Em Wed, Mar 13, 2019 at 09:53:01AM -0700, Andrii Nakryiko escreveu:
> btf_loader and ctf_loader didn't remove const/volatile/restrict, so
> bitfields using modifiers were not adjusted properly.
> 
> This patch abstracts logic of stripping aways typedefs and access
> modifiers into tag__strip_typedefs_and_modifiers, which handles any
> interleaving of typedefs and modifiers. dwarf_loader was adapter to
> reuse this function as well, instead of custom goto loop.

Thanks, applied.

- Arnaldo