[PATCH v3 0/6] Infer BTF alignment

[PATCH v3 0/6] Infer BTF alignment

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Infer alignment of struct members when loading BTF, so that using the
header dumped by pahole results in the same offsets. This is useful for
debugging tools to be able to access pointers to structs of possibly
private types. One such example are kernel modules bolting traceevents
on existing upstream tracepoints, which typically expose "top-level"
struct pointers (e.g. struct cfs_rq) to allow maximum flexibility.

Changes since v2:

* Added 3 commits on top to use cacheline size when infering larger
  alignments

Changes since v1:
* Split alignment patch into refactor and new code.
* Test with fullcircle on vmlinux and .o of kernel tree:
    
  export PATH=pahole/build:$PATH
  for f in {kernel,drivers,arch,block,crypto,certs,fs,init,ipc,lib,mm,net,sound,virt}/**.o; do
     echo $f
     pahole/fullcircle $f
  done

  This did not produce any error but neither did it on pahole master
  branch, since I assume it reads DWARF by default.

  Trying with "pfunct -F btf" on object files seemed to yield empty
  sources:

    pfunct: file 'kernel/sched/fair.o' has no btf type information.

  Running pfunct on vmlinux worked, but fullcircle seems to have bailed
  out before getting to pfunct. When commenting out the bailouts, I
  managed to get pfunct to work, but the header generated with --compile
  contains one duplicate type, and lots of broken inlined functions
  (e.g. "return 0;" in a func returning a struct).
  


Douglas Raillard (6):
  fprintf: Fix nested struct printing
  btf_loader.c: Refactor class__fixup_btf_bitfields
  btf_loader.c: Infer alignment info
  dwarves_fprintf: Move cacheline_size into struct conf_fprintf
  btf_loader.c: Propagate struct conf_load
  btf_loader.c: Use cacheline size to infer alignment

 btf_loader.c      | 86 ++++++++++++++++++++++++++++++++++++-----------
 codiff.c          |  4 ++-
 ctracer.c         |  3 +-
 dtagnames.c       |  3 +-
 dwarves.c         |  8 ++---
 dwarves.h         |  8 +++--
 dwarves_fprintf.c | 52 +++++++++++++++++++---------
 pahole.c          |  8 +++--
 pdwtags.c         |  4 ++-
 pfunct.c          |  4 ++-
 pglobal.c         |  4 ++-
 prefcnt.c         |  4 ++-
 12 files changed, 135 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] fprintf: Fix nested struct printing

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

This code:

    struct X {
       struct {
       } __attribute__((foo)) x __attribute__((bar));
    }

Was wrongly printed as:

    struct X {
       struct {
       } x __attribute__((foo)) __attribute__((bar));
    }

This unfortunately matters a lot, since "bar" is suppose to apply to
"x", but "foo" to typeof(x). In the wrong form, both apply to "x",
leading to e.g. incorrect layout for __aligned__ attribute.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 dwarves_fprintf.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index c35868a..1c1d949 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -787,7 +787,7 @@ print_default:
 					   (type->tag == DW_TAG_class_type &&
 					    !tconf.classes_as_structs) ? "class" : "struct",
 					   tconf.type_spacing - 7,
-					   type__name(ctype), name);
+					   type__name(ctype), name ?: "");
 		} else {
 			struct class *cclass = tag__class(type);
 
@@ -802,7 +802,7 @@ print_default:
 		ctype = tag__type(type);
 
 		if (type__name(ctype) != NULL && !expand_types) {
-			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name);
+			printed += fprintf(fp, "union %-*s %s", tconf.type_spacing - 6, type__name(ctype), name ?: "");
 		} else {
 			tconf.type_spacing -= 8;
 			printed += union__fprintf(ctype, cu, &tconf, fp);
@@ -812,7 +812,7 @@ print_default:
 		ctype = tag__type(type);
 
 		if (type__name(ctype) != NULL)
-			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name);
+			printed += fprintf(fp, "enum %-*s %s", tconf.type_spacing - 5, type__name(ctype), name ?: "");
 		else
 			printed += enumeration__fprintf(type, &tconf, fp);
 		break;
@@ -863,7 +863,21 @@ static size_t class_member__fprintf(struct class_member *member, bool union_memb
 	if (member->is_static)
 		printed += fprintf(fp, "static ");
 
-	printed += type__fprintf(type, cu, name, &sconf, fp);
+	/* For struct-like constructs, the name of the member cannot be
+	 * conflated with the name of its type, otherwise __attribute__ are
+	 * printed in the wrong order.
+	 */
+	if (tag__is_union(type) || tag__is_struct(type) ||
+	    tag__is_enumeration(type)) {
+		printed += type__fprintf(type, cu, NULL, &sconf, fp);
+		if (name) {
+			if (!type__name(tag__type(type)))
+				printed += fprintf(fp, " ");
+			printed += fprintf(fp, "%s", name);
+		}
+	} else {
+		printed += type__fprintf(type, cu, name, &sconf, fp);
+	}
 
 	if (member->is_static) {
 		if (member->const_value != 0)
-- 
2.25.1

Re: [PATCH v3 1/6] fprintf: Fix nested struct printing

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:05PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> This code:
> 
>     struct X {
>        struct {
>        } __attribute__((foo)) x __attribute__((bar));
>     }
> 
> Was wrongly printed as:
> 
>     struct X {
>        struct {
>        } x __attribute__((foo)) __attribute__((bar));
>     }

This one is already in the 'next' branch, i.e. was already applied.

- Arnaldo

[PATCH v3 2/6] btf_loader.c: Refactor class__fixup_btf_bitfields

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Refactor class__fixup_btf_bitfields to remove a "continue" statement, to
prepare the ground for alignment fixup that is relevant for some types
matching:

    type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 btf_loader.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 3e5a945..9c2daee 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -486,28 +486,27 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 		pos->byte_size = tag__size(type, cu);
 		pos->bit_size = pos->byte_size * 8;
 
-		/* bitfield fixup is needed for enums and base types only */
-		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
-			continue;
-
 		/* if BTF data is incorrect and has size == 0, skip field,
 		 * instead of crashing */
 		if (pos->byte_size == 0) {
 			continue;
 		}
 
-		if (pos->bitfield_size) {
-			/* bitfields seem to be always aligned, no matter the packing */
-			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
-			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
-			/* re-adjust bitfield offset if it is negative */
-			if (pos->bitfield_offset < 0) {
-				pos->bitfield_offset += pos->bit_size;
-				pos->byte_offset -= pos->byte_size;
-				pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
+		/* bitfield fixup is needed for enums and base types only */
+		if (type->tag == DW_TAG_base_type || type->tag == DW_TAG_enumeration_type) {
+			if (pos->bitfield_size) {
+				/* bitfields seem to be always aligned, no matter the packing */
+				pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
+				pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
+				/* re-adjust bitfield offset if it is negative */
+				if (pos->bitfield_offset < 0) {
+					pos->bitfield_offset += pos->bit_size;
+					pos->byte_offset -= pos->byte_size;
+					pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
+				}
+			} else {
+				pos->byte_offset = pos->bit_offset / 8;
 			}
-		} else {
-			pos->byte_offset = pos->bit_offset / 8;
 		}
 	}
 
-- 
2.25.1

Re: [PATCH v3 2/6] btf_loader.c: Refactor class__fixup_btf_bitfields

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:06PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Refactor class__fixup_btf_bitfields to remove a "continue" statement, to
> prepare the ground for alignment fixup that is relevant for some types
> matching:
> 
>     type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type

Also already applied.

- Arnaldo
 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  btf_loader.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 3e5a945..9c2daee 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -486,28 +486,27 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  		pos->byte_size = tag__size(type, cu);
>  		pos->bit_size = pos->byte_size * 8;
>  
> -		/* bitfield fixup is needed for enums and base types only */
> -		if (type->tag != DW_TAG_base_type && type->tag != DW_TAG_enumeration_type)
> -			continue;
> -
>  		/* if BTF data is incorrect and has size == 0, skip field,
>  		 * instead of crashing */
>  		if (pos->byte_size == 0) {
>  			continue;
>  		}
>  
> -		if (pos->bitfield_size) {
> -			/* bitfields seem to be always aligned, no matter the packing */
> -			pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
> -			pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
> -			/* re-adjust bitfield offset if it is negative */
> -			if (pos->bitfield_offset < 0) {
> -				pos->bitfield_offset += pos->bit_size;
> -				pos->byte_offset -= pos->byte_size;
> -				pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
> +		/* bitfield fixup is needed for enums and base types only */
> +		if (type->tag == DW_TAG_base_type || type->tag == DW_TAG_enumeration_type) {
> +			if (pos->bitfield_size) {
> +				/* bitfields seem to be always aligned, no matter the packing */
> +				pos->byte_offset = pos->bit_offset / pos->bit_size * pos->bit_size / 8;
> +				pos->bitfield_offset = pos->bit_offset - pos->byte_offset * 8;
> +				/* re-adjust bitfield offset if it is negative */
> +				if (pos->bitfield_offset < 0) {
> +					pos->bitfield_offset += pos->bit_size;
> +					pos->byte_offset -= pos->byte_size;
> +					pos->bit_offset = pos->byte_offset * 8 + pos->bitfield_offset;
> +				}
> +			} else {
> +				pos->byte_offset = pos->bit_offset / 8;
>  			}
> -		} else {
> -			pos->byte_offset = pos->bit_offset / 8;
>  		}
>  	}
>  
> -- 
> 2.25.1

-- 

- Arnaldo

[PATCH v3 3/6] btf_loader.c: Infer alignment info

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

BTF does not carry alignment information, but it carries the offset in
structs. This allows inferring the original alignment, yielding a C
header dump that is not identical to the original C code, but is
guaranteed to lead to the same memory layout.

This allows using the output of pahole in another program to poke at
memory, with the assurance that we will not read garbage.

Note: Since the alignment is inferred from the offset, it sometimes
happens that the offset was already correctly aligned, which means the
inferred alignment will be smaller than in the original source. This
does not impact the ability to read existing structs, but it could
impact creating such struct if other client code expects higher
alignment than the one exposed in the generated header.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 btf_loader.c | 36 ++++++++++++++++++++++++++++++++++++
 dwarves.c    |  2 +-
 dwarves.h    |  2 ++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/btf_loader.c b/btf_loader.c
index 9c2daee..2885252 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -471,10 +471,37 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
 	return btf__load_types(btf, cu);
 }
 
+static uint32_t class__infer_alignment(uint32_t byte_offset,
+				       uint32_t natural_alignment,
+				       uint32_t smallest_offset)
+{
+	uint32_t alignment = 0;
+	uint32_t offset_delta = byte_offset - smallest_offset;
+
+	if (offset_delta) {
+		if (byte_offset % 2 == 0) {
+			/* Find the power of 2 immediately higher than
+			 * offset_delta
+			 */
+			alignment = 1 << (8 * sizeof(offset_delta) -
+					      __builtin_clz(offset_delta));
+		} else {
+			alignment = 0;
+		}
+	}
+
+	/* Natural alignment, nothing to do */
+	if (alignment <= natural_alignment || alignment == 1)
+		alignment = 0;
+
+	return alignment;
+}
+
 static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 {
 	struct class_member *pos;
 	struct type *tag_type = tag__type(tag);
+	uint32_t smallest_offset = 0;
 
 	type__for_each_data_member(tag_type, pos) {
 		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
@@ -508,8 +535,17 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 				pos->byte_offset = pos->bit_offset / 8;
 			}
 		}
+
+		pos->alignment = class__infer_alignment(pos->byte_offset,
+							tag__natural_alignment(type, cu),
+							smallest_offset);
+		smallest_offset = pos->byte_offset + pos->byte_size;
 	}
 
+	tag_type->alignment = class__infer_alignment(tag_type->size,
+						     tag__natural_alignment(tag, cu),
+						     smallest_offset);
+
 	return 0;
 }
 
diff --git a/dwarves.c b/dwarves.c
index b6f2489..bb8af5b 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -1515,7 +1515,7 @@ void class__find_holes(struct class *class)
 
 static size_t type__natural_alignment(struct type *type, const struct cu *cu);
 
-static size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
+size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
 {
 	size_t natural_alignment = 1;
 
diff --git a/dwarves.h b/dwarves.h
index 30d33fa..c2fea0a 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -1002,6 +1002,8 @@ struct type {
 
 void __type__init(struct type *type);
 
+size_t tag__natural_alignment(struct tag *tag, const struct cu *cu);
+
 static inline struct class *type__class(const struct type *type)
 {
 	return (struct class *)type;
-- 
2.25.1

Re: [PATCH v3 3/6] btf_loader.c: Infer alignment info

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:07PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> BTF does not carry alignment information, but it carries the offset in
> structs. This allows inferring the original alignment, yielding a C
> header dump that is not identical to the original C code, but is
> guaranteed to lead to the same memory layout.
> 
> This allows using the output of pahole in another program to poke at
> memory, with the assurance that we will not read garbage.
> 
> Note: Since the alignment is inferred from the offset, it sometimes
> happens that the offset was already correctly aligned, which means the
> inferred alignment will be smaller than in the original source. This
> does not impact the ability to read existing structs, but it could
> impact creating such struct if other client code expects higher
> alignment than the one exposed in the generated header.

This as well was already applied, I split it in two:

"core: Export tag__natural_alignment()"
https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=next&id=4db65fe0cd02b3cc49d4fc8ff6c4c21a9ddb3642

"btf_loader: Infer alignment info"
https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=next&id=836c139fdf6f2b13ec2e5513df881272bb78aeb4
 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  btf_loader.c | 36 ++++++++++++++++++++++++++++++++++++
>  dwarves.c    |  2 +-
>  dwarves.h    |  2 ++
>  3 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 9c2daee..2885252 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -471,10 +471,37 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
>  	return btf__load_types(btf, cu);
>  }
>  
> +static uint32_t class__infer_alignment(uint32_t byte_offset,
> +				       uint32_t natural_alignment,
> +				       uint32_t smallest_offset)
> +{
> +	uint32_t alignment = 0;
> +	uint32_t offset_delta = byte_offset - smallest_offset;
> +
> +	if (offset_delta) {
> +		if (byte_offset % 2 == 0) {
> +			/* Find the power of 2 immediately higher than
> +			 * offset_delta
> +			 */
> +			alignment = 1 << (8 * sizeof(offset_delta) -
> +					      __builtin_clz(offset_delta));
> +		} else {
> +			alignment = 0;
> +		}
> +	}
> +
> +	/* Natural alignment, nothing to do */
> +	if (alignment <= natural_alignment || alignment == 1)
> +		alignment = 0;
> +
> +	return alignment;
> +}
> +
>  static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  {
>  	struct class_member *pos;
>  	struct type *tag_type = tag__type(tag);
> +	uint32_t smallest_offset = 0;
>  
>  	type__for_each_data_member(tag_type, pos) {
>  		struct tag *type = tag__strip_typedefs_and_modifiers(&pos->tag, cu);
> @@ -508,8 +535,17 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  				pos->byte_offset = pos->bit_offset / 8;
>  			}
>  		}
> +
> +		pos->alignment = class__infer_alignment(pos->byte_offset,
> +							tag__natural_alignment(type, cu),
> +							smallest_offset);
> +		smallest_offset = pos->byte_offset + pos->byte_size;
>  	}
>  
> +	tag_type->alignment = class__infer_alignment(tag_type->size,
> +						     tag__natural_alignment(tag, cu),
> +						     smallest_offset);
> +
>  	return 0;
>  }
>  
> diff --git a/dwarves.c b/dwarves.c
> index b6f2489..bb8af5b 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -1515,7 +1515,7 @@ void class__find_holes(struct class *class)
>  
>  static size_t type__natural_alignment(struct type *type, const struct cu *cu);
>  
> -static size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu)
>  {
>  	size_t natural_alignment = 1;
>  
> diff --git a/dwarves.h b/dwarves.h
> index 30d33fa..c2fea0a 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -1002,6 +1002,8 @@ struct type {
>  
>  void __type__init(struct type *type);
>  
> +size_t tag__natural_alignment(struct tag *tag, const struct cu *cu);
> +
>  static inline struct class *type__class(const struct type *type)
>  {
>  	return (struct class *)type;
> -- 
> 2.25.1

-- 

- Arnaldo

[PATCH v3 4/6] dwarves_fprintf: Move cacheline_size into struct conf_fprintf

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Remove the global variable and turn it into a member in struct
conf_fprintf, so that it can be used by other parts of the code.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 codiff.c          |  4 +++-
 ctracer.c         |  3 ++-
 dtagnames.c       |  3 ++-
 dwarves.c         |  6 +-----
 dwarves.h         |  6 ++++--
 dwarves_fprintf.c | 30 ++++++++++++++++++------------
 pahole.c          |  8 +++++---
 pdwtags.c         |  4 +++-
 pfunct.c          |  4 +++-
 pglobal.c         |  4 +++-
 prefcnt.c         |  4 +++-
 11 files changed, 47 insertions(+), 29 deletions(-)

diff --git a/codiff.c b/codiff.c
index 13a94c1..35aee3f 100644
--- a/codiff.c
+++ b/codiff.c
@@ -778,11 +778,13 @@ failure:
 		goto out;
 	}
 
-	if (dwarves__init(0)) {
+	if (dwarves__init()) {
 		fputs("codiff: insufficient memory\n", stderr);
 		goto out;
 	}
 
+	dwarves__resolve_cacheline_size(&conf_load, 0);
+
 	if (show_function_diffs == 0 && show_struct_diffs == 0 &&
 	    show_terse_type_changes == 0)
 		show_function_diffs = show_struct_diffs = 1;
diff --git a/ctracer.c b/ctracer.c
index d0b2623..10ecac6 100644
--- a/ctracer.c
+++ b/ctracer.c
@@ -940,10 +940,11 @@ int main(int argc, char *argv[])
 	FILE *fp_functions;
 	int rc = EXIT_FAILURE;
 
-	if (dwarves__init(0)) {
+	if (dwarves__init()) {
 		fputs("ctracer: insufficient memory\n", stderr);
 		goto out;
 	}
+	dwarves__resolve_cacheline_size(NULL, 0);
 
 	if (argp_parse(&ctracer__argp, argc, argv, 0, &remaining, NULL) ||
 	    remaining < argc) {
diff --git a/dtagnames.c b/dtagnames.c
index 6cb51f1..343f055 100644
--- a/dtagnames.c
+++ b/dtagnames.c
@@ -34,10 +34,11 @@ int main(int argc __maybe_unused, char *argv[])
 	int err, rc = EXIT_FAILURE;
 	struct cus *cus = cus__new();
 
-	if (dwarves__init(0) || cus == NULL) {
+	if (dwarves__init() || cus == NULL) {
 		fputs("dtagnames: insufficient memory\n", stderr);
 		goto out;
 	}
+	dwarves__resolve_cacheline_size(NULL, 0);
 
 	err = cus__load_files(cus, NULL, argv + 1);
 	if (err != 0) {
diff --git a/dwarves.c b/dwarves.c
index bb8af5b..81fa47b 100644
--- a/dwarves.c
+++ b/dwarves.c
@@ -2458,12 +2458,8 @@ void cus__set_loader_exit(struct cus *cus, void (*loader_exit)(struct cus *cus))
 	cus->loader_exit = loader_exit;
 }
 
-void dwarves__fprintf_init(uint16_t user_cacheline_size);
-
-int dwarves__init(uint16_t user_cacheline_size)
+int dwarves__init(void)
 {
-	dwarves__fprintf_init(user_cacheline_size);
-
 	int i = 0;
 	int err = 0;
 
diff --git a/dwarves.h b/dwarves.h
index c2fea0a..6ad355d 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -96,6 +96,7 @@ struct conf_fprintf {
 	const char *header_type;
 	const char *range;
 	uint32_t   skip;
+	uint16_t   cacheline_size;
 	uint8_t	   indent;
 	uint8_t	   expand_types:1;
 	uint8_t	   expand_pointers:1;
@@ -569,7 +570,7 @@ void tag__not_found_die(const char *file, int line, const char *func);
 					  __LINE__, __func__); } while (0)
 
 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);
+size_t tag__nr_cachelines(const struct conf_fprintf *conf, 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);
 
@@ -1331,8 +1332,9 @@ void enumeration__add(struct type *type, struct enumerator *enumerator);
 size_t enumeration__fprintf(const struct tag *tag_enum,
 			    const struct conf_fprintf *conf, FILE *fp);
 
-int dwarves__init(uint16_t user_cacheline_size);
+int dwarves__init(void);
 void dwarves__exit(void);
+void dwarves__resolve_cacheline_size(const struct conf_load *conf, uint16_t user_cacheline_size);
 
 const char *dwarf_tag_name(const uint32_t tag);
 
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index 1c1d949..efccd90 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -127,7 +127,7 @@ const char *dwarf_tag_name(const uint32_t tag)
 	return "INVALID";
 }
 
-static const struct conf_fprintf conf_fprintf__defaults = {
+static struct conf_fprintf conf_fprintf__defaults = {
 	.name_spacing = 23,
 	.type_spacing = 26,
 	.emit_stats   = 1,
@@ -135,11 +135,10 @@ static const struct conf_fprintf conf_fprintf__defaults = {
 
 const char tabs[] = "\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t";
 
-static size_t cacheline_size;
 
-size_t tag__nr_cachelines(const struct tag *tag, const struct cu *cu)
+size_t tag__nr_cachelines(const struct conf_fprintf *conf, const struct tag *tag, const struct cu *cu)
 {
-	return (tag__size(tag, cu) + cacheline_size - 1) / cacheline_size;
+	return (tag__size(tag, cu) + conf->cacheline_size - 1) / conf->cacheline_size;
 }
 
 static const char *tag__accessibility(const struct tag *tag)
@@ -611,7 +610,7 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
 {
 	size_t printed = fprintf(fp, "\n%.*s/* size: %d, cachelines: %zd, members: %u",
 				 conf->indent, tabs, type->size,
-				 tag__nr_cachelines(type__tag(type), cu), type->nr_members);
+				 tag__nr_cachelines(conf, type__tag(type), cu), type->nr_members);
 
 	if (type->nr_static_members != 0)
 		printed += fprintf(fp, ", static members: %u */\n", type->nr_static_members);
@@ -1307,11 +1306,11 @@ static size_t class__fprintf_cacheline_boundary(struct conf_fprintf *conf,
 						FILE *fp)
 {
 	int indent = conf->indent;
-	uint32_t cacheline = offset / cacheline_size;
+	uint32_t cacheline = offset / conf->cacheline_size;
 	size_t printed = 0;
 
 	if (cacheline > *conf->cachelinep) {
-		const uint32_t cacheline_pos = offset % cacheline_size;
+		const uint32_t cacheline_pos = offset % conf->cacheline_size;
 		const uint32_t cacheline_in_bytes = offset - cacheline_pos;
 
 		if (cacheline_pos == 0)
@@ -1753,7 +1752,7 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
 		}
 		printed += fprintf(fp, " */\n");
 	}
-	cacheline = (cconf.base_offset + type->size) % cacheline_size;
+	cacheline = (cconf.base_offset + type->size) % conf->cacheline_size;
 	if (cacheline != 0)
 		printed += fprintf(fp, "%.*s/* last cacheline: %u bytes */\n",
 				   cconf.indent, tabs,
@@ -1980,15 +1979,22 @@ static long cacheline__size(void)
 #endif
 }
 
-void dwarves__fprintf_init(uint16_t user_cacheline_size)
+void dwarves__resolve_cacheline_size(const struct conf_load *conf, uint16_t user_cacheline_size)
 {
+	uint16_t size;
+
 	if (user_cacheline_size == 0) {
 		long sys_cacheline_size = cacheline__size();
 
 		if (sys_cacheline_size > 0)
-			cacheline_size = sys_cacheline_size;
+			size = sys_cacheline_size;
 		else
-			cacheline_size = 64; /* Fall back to a sane value */
+			size = 64; /* Fall back to a sane value */
 	} else
-		cacheline_size = user_cacheline_size;
+		size = user_cacheline_size;
+
+	if (conf)
+		conf->conf_fprintf->cacheline_size = size;
+
+	conf_fprintf__defaults.cacheline_size = size;
 }
diff --git a/pahole.c b/pahole.c
index 80271b5..6ab2f80 100644
--- a/pahole.c
+++ b/pahole.c
@@ -1684,8 +1684,8 @@ static void do_reorg(struct tag *class, struct cu *cu)
 	tag__fprintf(class__tag(clone), cu, &conf, stdout);
 	if (savings != 0) {
 		const size_t cacheline_savings =
-		      (tag__nr_cachelines(class, cu) -
-		       tag__nr_cachelines(class__tag(clone), cu));
+		      (tag__nr_cachelines(&conf, class, cu) -
+		       tag__nr_cachelines(&conf, class__tag(clone), cu));
 
 		printf("   /* saved %d byte%s", savings,
 		       savings != 1 ? "s" : "");
@@ -3143,11 +3143,13 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
-	if (dwarves__init(cacheline_size)) {
+	if (dwarves__init()) {
 		fputs("pahole: insufficient memory\n", stderr);
 		goto out;
 	}
 
+	dwarves__resolve_cacheline_size(&conf_load, cacheline_size);
+
 	if (prettify_input_filename) {
 		if (strcmp(prettify_input_filename, "-") == 0) {
 			prettify_input = stdin;
diff --git a/pdwtags.c b/pdwtags.c
index 270ddc4..2b5ba1b 100644
--- a/pdwtags.c
+++ b/pdwtags.c
@@ -131,11 +131,13 @@ int main(int argc, char *argv[])
 	int remaining, rc = EXIT_FAILURE, err;
 	struct cus *cus = cus__new();
 
-	if (dwarves__init(0) || cus == NULL) {
+	if (dwarves__init() || cus == NULL) {
 		fputs("pwdtags: insufficient memory\n", stderr);
 		goto out;
 	}
 
+	dwarves__resolve_cacheline_size(&pdwtags_conf_load, 0);
+
 	if (argp_parse(&pdwtags__argp, argc, argv, 0, &remaining, NULL) ||
 	    remaining == argc) {
                 argp_help(&pdwtags__argp, stderr, ARGP_HELP_SEE, argv[0]);
diff --git a/pfunct.c b/pfunct.c
index e69c9cd..5485622 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -721,11 +721,13 @@ int main(int argc, char *argv[])
 	if (symtab_name != NULL)
 		return elf_symtabs__show(argv + remaining);
 
-	if (dwarves__init(0)) {
+	if (dwarves__init()) {
 		fputs("pfunct: insufficient memory\n", stderr);
 		goto out;
 	}
 
+	dwarves__resolve_cacheline_size(&conf_load, 0);
+
 	struct cus *cus = cus__new();
 	if (cus == NULL) {
 		fputs("pfunct: insufficient memory\n", stderr);
diff --git a/pglobal.c b/pglobal.c
index 516d3f0..9341244 100644
--- a/pglobal.c
+++ b/pglobal.c
@@ -303,11 +303,13 @@ int main(int argc, char *argv[])
                 goto out;
 	}
 
-	if (dwarves__init(0)) {
+	if (dwarves__init()) {
 		fputs("pglobal: insufficient memory\n", stderr);
 		goto out;
 	}
 
+	dwarves__resolve_cacheline_size(&conf_load, 0);
+
 	struct cus *cus = cus__new();
 	if (cus == NULL) {
 		fputs("pglobal: insufficient memory\n", stderr);
diff --git a/prefcnt.c b/prefcnt.c
index b37192f..8010afd 100644
--- a/prefcnt.c
+++ b/prefcnt.c
@@ -136,11 +136,13 @@ int main(int argc __maybe_unused, char *argv[])
 	int err;
 	struct cus *cus = cus__new();
 
-	if (dwarves__init(0) || cus == NULL) {
+	if (dwarves__init() || cus == NULL) {
 		fputs("prefcnt: insufficient memory\n", stderr);
 		return EXIT_FAILURE;
 	}
 
+	dwarves__resolve_cacheline_size(NULL, 0);
+
 	err = cus__load_files(cus, NULL, argv + 1);
 	if (err != 0) {
 		cus__fprintf_load_files_err(cus, "prefcnt", argv + 1, err, stderr);
-- 
2.25.1

Re: [PATCH v3 4/6] dwarves_fprintf: Move cacheline_size into struct conf_fprintf

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:08PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Remove the global variable and turn it into a member in struct
> conf_fprintf, so that it can be used by other parts of the code.

Thanks, applied.

- Arnaldo

 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  codiff.c          |  4 +++-
>  ctracer.c         |  3 ++-
>  dtagnames.c       |  3 ++-
>  dwarves.c         |  6 +-----
>  dwarves.h         |  6 ++++--
>  dwarves_fprintf.c | 30 ++++++++++++++++++------------
>  pahole.c          |  8 +++++---
>  pdwtags.c         |  4 +++-
>  pfunct.c          |  4 +++-
>  pglobal.c         |  4 +++-
>  prefcnt.c         |  4 +++-
>  11 files changed, 47 insertions(+), 29 deletions(-)
> 
> diff --git a/codiff.c b/codiff.c
> index 13a94c1..35aee3f 100644
> --- a/codiff.c
> +++ b/codiff.c
> @@ -778,11 +778,13 @@ failure:
>  		goto out;
>  	}
>  
> -	if (dwarves__init(0)) {
> +	if (dwarves__init()) {
>  		fputs("codiff: insufficient memory\n", stderr);
>  		goto out;
>  	}
>  
> +	dwarves__resolve_cacheline_size(&conf_load, 0);
> +
>  	if (show_function_diffs == 0 && show_struct_diffs == 0 &&
>  	    show_terse_type_changes == 0)
>  		show_function_diffs = show_struct_diffs = 1;
> diff --git a/ctracer.c b/ctracer.c
> index d0b2623..10ecac6 100644
> --- a/ctracer.c
> +++ b/ctracer.c
> @@ -940,10 +940,11 @@ int main(int argc, char *argv[])
>  	FILE *fp_functions;
>  	int rc = EXIT_FAILURE;
>  
> -	if (dwarves__init(0)) {
> +	if (dwarves__init()) {
>  		fputs("ctracer: insufficient memory\n", stderr);
>  		goto out;
>  	}
> +	dwarves__resolve_cacheline_size(NULL, 0);
>  
>  	if (argp_parse(&ctracer__argp, argc, argv, 0, &remaining, NULL) ||
>  	    remaining < argc) {
> diff --git a/dtagnames.c b/dtagnames.c
> index 6cb51f1..343f055 100644
> --- a/dtagnames.c
> +++ b/dtagnames.c
> @@ -34,10 +34,11 @@ int main(int argc __maybe_unused, char *argv[])
>  	int err, rc = EXIT_FAILURE;
>  	struct cus *cus = cus__new();
>  
> -	if (dwarves__init(0) || cus == NULL) {
> +	if (dwarves__init() || cus == NULL) {
>  		fputs("dtagnames: insufficient memory\n", stderr);
>  		goto out;
>  	}
> +	dwarves__resolve_cacheline_size(NULL, 0);
>  
>  	err = cus__load_files(cus, NULL, argv + 1);
>  	if (err != 0) {
> diff --git a/dwarves.c b/dwarves.c
> index bb8af5b..81fa47b 100644
> --- a/dwarves.c
> +++ b/dwarves.c
> @@ -2458,12 +2458,8 @@ void cus__set_loader_exit(struct cus *cus, void (*loader_exit)(struct cus *cus))
>  	cus->loader_exit = loader_exit;
>  }
>  
> -void dwarves__fprintf_init(uint16_t user_cacheline_size);
> -
> -int dwarves__init(uint16_t user_cacheline_size)
> +int dwarves__init(void)
>  {
> -	dwarves__fprintf_init(user_cacheline_size);
> -
>  	int i = 0;
>  	int err = 0;
>  
> diff --git a/dwarves.h b/dwarves.h
> index c2fea0a..6ad355d 100644
> --- a/dwarves.h
> +++ b/dwarves.h
> @@ -96,6 +96,7 @@ struct conf_fprintf {
>  	const char *header_type;
>  	const char *range;
>  	uint32_t   skip;
> +	uint16_t   cacheline_size;
>  	uint8_t	   indent;
>  	uint8_t	   expand_types:1;
>  	uint8_t	   expand_pointers:1;
> @@ -569,7 +570,7 @@ void tag__not_found_die(const char *file, int line, const char *func);
>  					  __LINE__, __func__); } while (0)
>  
>  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);
> +size_t tag__nr_cachelines(const struct conf_fprintf *conf, 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);
>  
> @@ -1331,8 +1332,9 @@ void enumeration__add(struct type *type, struct enumerator *enumerator);
>  size_t enumeration__fprintf(const struct tag *tag_enum,
>  			    const struct conf_fprintf *conf, FILE *fp);
>  
> -int dwarves__init(uint16_t user_cacheline_size);
> +int dwarves__init(void);
>  void dwarves__exit(void);
> +void dwarves__resolve_cacheline_size(const struct conf_load *conf, uint16_t user_cacheline_size);
>  
>  const char *dwarf_tag_name(const uint32_t tag);
>  
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 1c1d949..efccd90 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -127,7 +127,7 @@ const char *dwarf_tag_name(const uint32_t tag)
>  	return "INVALID";
>  }
>  
> -static const struct conf_fprintf conf_fprintf__defaults = {
> +static struct conf_fprintf conf_fprintf__defaults = {
>  	.name_spacing = 23,
>  	.type_spacing = 26,
>  	.emit_stats   = 1,
> @@ -135,11 +135,10 @@ static const struct conf_fprintf conf_fprintf__defaults = {
>  
>  const char tabs[] = "\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t";
>  
> -static size_t cacheline_size;
>  
> -size_t tag__nr_cachelines(const struct tag *tag, const struct cu *cu)
> +size_t tag__nr_cachelines(const struct conf_fprintf *conf, const struct tag *tag, const struct cu *cu)
>  {
> -	return (tag__size(tag, cu) + cacheline_size - 1) / cacheline_size;
> +	return (tag__size(tag, cu) + conf->cacheline_size - 1) / conf->cacheline_size;
>  }
>  
>  static const char *tag__accessibility(const struct tag *tag)
> @@ -611,7 +610,7 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
>  {
>  	size_t printed = fprintf(fp, "\n%.*s/* size: %d, cachelines: %zd, members: %u",
>  				 conf->indent, tabs, type->size,
> -				 tag__nr_cachelines(type__tag(type), cu), type->nr_members);
> +				 tag__nr_cachelines(conf, type__tag(type), cu), type->nr_members);
>  
>  	if (type->nr_static_members != 0)
>  		printed += fprintf(fp, ", static members: %u */\n", type->nr_static_members);
> @@ -1307,11 +1306,11 @@ static size_t class__fprintf_cacheline_boundary(struct conf_fprintf *conf,
>  						FILE *fp)
>  {
>  	int indent = conf->indent;
> -	uint32_t cacheline = offset / cacheline_size;
> +	uint32_t cacheline = offset / conf->cacheline_size;
>  	size_t printed = 0;
>  
>  	if (cacheline > *conf->cachelinep) {
> -		const uint32_t cacheline_pos = offset % cacheline_size;
> +		const uint32_t cacheline_pos = offset % conf->cacheline_size;
>  		const uint32_t cacheline_in_bytes = offset - cacheline_pos;
>  
>  		if (cacheline_pos == 0)
> @@ -1753,7 +1752,7 @@ static size_t __class__fprintf(struct class *class, const struct cu *cu,
>  		}
>  		printed += fprintf(fp, " */\n");
>  	}
> -	cacheline = (cconf.base_offset + type->size) % cacheline_size;
> +	cacheline = (cconf.base_offset + type->size) % conf->cacheline_size;
>  	if (cacheline != 0)
>  		printed += fprintf(fp, "%.*s/* last cacheline: %u bytes */\n",
>  				   cconf.indent, tabs,
> @@ -1980,15 +1979,22 @@ static long cacheline__size(void)
>  #endif
>  }
>  
> -void dwarves__fprintf_init(uint16_t user_cacheline_size)
> +void dwarves__resolve_cacheline_size(const struct conf_load *conf, uint16_t user_cacheline_size)
>  {
> +	uint16_t size;
> +
>  	if (user_cacheline_size == 0) {
>  		long sys_cacheline_size = cacheline__size();
>  
>  		if (sys_cacheline_size > 0)
> -			cacheline_size = sys_cacheline_size;
> +			size = sys_cacheline_size;
>  		else
> -			cacheline_size = 64; /* Fall back to a sane value */
> +			size = 64; /* Fall back to a sane value */
>  	} else
> -		cacheline_size = user_cacheline_size;
> +		size = user_cacheline_size;
> +
> +	if (conf)
> +		conf->conf_fprintf->cacheline_size = size;
> +
> +	conf_fprintf__defaults.cacheline_size = size;
>  }
> diff --git a/pahole.c b/pahole.c
> index 80271b5..6ab2f80 100644
> --- a/pahole.c
> +++ b/pahole.c
> @@ -1684,8 +1684,8 @@ static void do_reorg(struct tag *class, struct cu *cu)
>  	tag__fprintf(class__tag(clone), cu, &conf, stdout);
>  	if (savings != 0) {
>  		const size_t cacheline_savings =
> -		      (tag__nr_cachelines(class, cu) -
> -		       tag__nr_cachelines(class__tag(clone), cu));
> +		      (tag__nr_cachelines(&conf, class, cu) -
> +		       tag__nr_cachelines(&conf, class__tag(clone), cu));
>  
>  		printf("   /* saved %d byte%s", savings,
>  		       savings != 1 ? "s" : "");
> @@ -3143,11 +3143,13 @@ int main(int argc, char *argv[])
>  		goto out;
>  	}
>  
> -	if (dwarves__init(cacheline_size)) {
> +	if (dwarves__init()) {
>  		fputs("pahole: insufficient memory\n", stderr);
>  		goto out;
>  	}
>  
> +	dwarves__resolve_cacheline_size(&conf_load, cacheline_size);
> +
>  	if (prettify_input_filename) {
>  		if (strcmp(prettify_input_filename, "-") == 0) {
>  			prettify_input = stdin;
> diff --git a/pdwtags.c b/pdwtags.c
> index 270ddc4..2b5ba1b 100644
> --- a/pdwtags.c
> +++ b/pdwtags.c
> @@ -131,11 +131,13 @@ int main(int argc, char *argv[])
>  	int remaining, rc = EXIT_FAILURE, err;
>  	struct cus *cus = cus__new();
>  
> -	if (dwarves__init(0) || cus == NULL) {
> +	if (dwarves__init() || cus == NULL) {
>  		fputs("pwdtags: insufficient memory\n", stderr);
>  		goto out;
>  	}
>  
> +	dwarves__resolve_cacheline_size(&pdwtags_conf_load, 0);
> +
>  	if (argp_parse(&pdwtags__argp, argc, argv, 0, &remaining, NULL) ||
>  	    remaining == argc) {
>                  argp_help(&pdwtags__argp, stderr, ARGP_HELP_SEE, argv[0]);
> diff --git a/pfunct.c b/pfunct.c
> index e69c9cd..5485622 100644
> --- a/pfunct.c
> +++ b/pfunct.c
> @@ -721,11 +721,13 @@ int main(int argc, char *argv[])
>  	if (symtab_name != NULL)
>  		return elf_symtabs__show(argv + remaining);
>  
> -	if (dwarves__init(0)) {
> +	if (dwarves__init()) {
>  		fputs("pfunct: insufficient memory\n", stderr);
>  		goto out;
>  	}
>  
> +	dwarves__resolve_cacheline_size(&conf_load, 0);
> +
>  	struct cus *cus = cus__new();
>  	if (cus == NULL) {
>  		fputs("pfunct: insufficient memory\n", stderr);
> diff --git a/pglobal.c b/pglobal.c
> index 516d3f0..9341244 100644
> --- a/pglobal.c
> +++ b/pglobal.c
> @@ -303,11 +303,13 @@ int main(int argc, char *argv[])
>                  goto out;
>  	}
>  
> -	if (dwarves__init(0)) {
> +	if (dwarves__init()) {
>  		fputs("pglobal: insufficient memory\n", stderr);
>  		goto out;
>  	}
>  
> +	dwarves__resolve_cacheline_size(&conf_load, 0);
> +
>  	struct cus *cus = cus__new();
>  	if (cus == NULL) {
>  		fputs("pglobal: insufficient memory\n", stderr);
> diff --git a/prefcnt.c b/prefcnt.c
> index b37192f..8010afd 100644
> --- a/prefcnt.c
> +++ b/prefcnt.c
> @@ -136,11 +136,13 @@ int main(int argc __maybe_unused, char *argv[])
>  	int err;
>  	struct cus *cus = cus__new();
>  
> -	if (dwarves__init(0) || cus == NULL) {
> +	if (dwarves__init() || cus == NULL) {
>  		fputs("prefcnt: insufficient memory\n", stderr);
>  		return EXIT_FAILURE;
>  	}
>  
> +	dwarves__resolve_cacheline_size(NULL, 0);
> +
>  	err = cus__load_files(cus, NULL, argv + 1);
>  	if (err != 0) {
>  		cus__fprintf_load_files_err(cus, "prefcnt", argv + 1, err, stderr);
> -- 
> 2.25.1

-- 

- Arnaldo

[PATCH v3 5/6] btf_loader.c: Propagate struct conf_load

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

Give access to struct conf_load in class__infer_alignment.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 btf_loader.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index 2885252..e500eae 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -471,7 +471,8 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
 	return btf__load_types(btf, cu);
 }
 
-static uint32_t class__infer_alignment(uint32_t byte_offset,
+static uint32_t class__infer_alignment(const struct conf_load *conf,
+				       uint32_t byte_offset,
 				       uint32_t natural_alignment,
 				       uint32_t smallest_offset)
 {
@@ -497,7 +498,7 @@ static uint32_t class__infer_alignment(uint32_t byte_offset,
 	return alignment;
 }
 
-static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
+static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *tag, struct cu *cu)
 {
 	struct class_member *pos;
 	struct type *tag_type = tag__type(tag);
@@ -536,27 +537,29 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
 			}
 		}
 
-		pos->alignment = class__infer_alignment(pos->byte_offset,
+		pos->alignment = class__infer_alignment(conf,
+							pos->byte_offset,
 							tag__natural_alignment(type, cu),
 							smallest_offset);
 		smallest_offset = pos->byte_offset + pos->byte_size;
 	}
 
-	tag_type->alignment = class__infer_alignment(tag_type->size,
+	tag_type->alignment = class__infer_alignment(conf,
+						     tag_type->size,
 						     tag__natural_alignment(tag, cu),
 						     smallest_offset);
 
 	return 0;
 }
 
-static int cu__fixup_btf_bitfields(struct cu *cu)
+static int cu__fixup_btf_bitfields(const struct conf_load *conf, struct cu *cu)
 {
 	int err = 0;
 	struct tag *pos;
 
 	list_for_each_entry(pos, &cu->tags, node)
 		if (tag__is_struct(pos) || tag__is_union(pos)) {
-			err = class__fixup_btf_bitfields(pos, cu);
+			err = class__fixup_btf_bitfields(conf, pos, cu);
 			if (err)
 				break;
 		}
@@ -606,7 +609,7 @@ static int cus__load_btf(struct cus *cus, struct conf_load *conf, const char *fi
 	if (err != 0)
 		goto out_free;
 
-	err = cu__fixup_btf_bitfields(cu);
+	err = cu__fixup_btf_bitfields(conf, cu);
 	/*
 	 * The app stole this cu, possibly deleting it,
 	 * so forget about it
-- 
2.25.1

Re: [PATCH v3 5/6] btf_loader.c: Propagate struct conf_load

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:09PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> Give access to struct conf_load in class__infer_alignment.

prep patch, paving the way, thanks for doing it like that. :-)

Applied.

- Arnaldo

 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  btf_loader.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index 2885252..e500eae 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -471,7 +471,8 @@ static int btf__load_sections(struct btf *btf, struct cu *cu)
>  	return btf__load_types(btf, cu);
>  }
>  
> -static uint32_t class__infer_alignment(uint32_t byte_offset,
> +static uint32_t class__infer_alignment(const struct conf_load *conf,
> +				       uint32_t byte_offset,
>  				       uint32_t natural_alignment,
>  				       uint32_t smallest_offset)
>  {
> @@ -497,7 +498,7 @@ static uint32_t class__infer_alignment(uint32_t byte_offset,
>  	return alignment;
>  }
>  
> -static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
> +static int class__fixup_btf_bitfields(const struct conf_load *conf, struct tag *tag, struct cu *cu)
>  {
>  	struct class_member *pos;
>  	struct type *tag_type = tag__type(tag);
> @@ -536,27 +537,29 @@ static int class__fixup_btf_bitfields(struct tag *tag, struct cu *cu)
>  			}
>  		}
>  
> -		pos->alignment = class__infer_alignment(pos->byte_offset,
> +		pos->alignment = class__infer_alignment(conf,
> +							pos->byte_offset,
>  							tag__natural_alignment(type, cu),
>  							smallest_offset);
>  		smallest_offset = pos->byte_offset + pos->byte_size;
>  	}
>  
> -	tag_type->alignment = class__infer_alignment(tag_type->size,
> +	tag_type->alignment = class__infer_alignment(conf,
> +						     tag_type->size,
>  						     tag__natural_alignment(tag, cu),
>  						     smallest_offset);
>  
>  	return 0;
>  }
>  
> -static int cu__fixup_btf_bitfields(struct cu *cu)
> +static int cu__fixup_btf_bitfields(const struct conf_load *conf, struct cu *cu)
>  {
>  	int err = 0;
>  	struct tag *pos;
>  
>  	list_for_each_entry(pos, &cu->tags, node)
>  		if (tag__is_struct(pos) || tag__is_union(pos)) {
> -			err = class__fixup_btf_bitfields(pos, cu);
> +			err = class__fixup_btf_bitfields(conf, pos, cu);
>  			if (err)
>  				break;
>  		}
> @@ -606,7 +609,7 @@ static int cus__load_btf(struct cus *cus, struct conf_load *conf, const char *fi
>  	if (err != 0)
>  		goto out_free;
>  
> -	err = cu__fixup_btf_bitfields(cu);
> +	err = cu__fixup_btf_bitfields(conf, cu);
>  	/*
>  	 * The app stole this cu, possibly deleting it,
>  	 * so forget about it
> -- 
> 2.25.1

-- 

- Arnaldo

[PATCH v3 6/6] btf_loader.c: Use cacheline size to infer alignment

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

When the alignment is larger than natural, it is very likely that the
source code was using the cacheline size. Therefore, use the cacheline
size when it would only result in increasing the alignment.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 btf_loader.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/btf_loader.c b/btf_loader.c
index e500eae..7a5b16f 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -476,6 +476,7 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
 				       uint32_t natural_alignment,
 				       uint32_t smallest_offset)
 {
+	uint16_t cacheline_size = conf->conf_fprintf->cacheline_size;
 	uint32_t alignment = 0;
 	uint32_t offset_delta = byte_offset - smallest_offset;
 
@@ -494,6 +495,15 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
 	/* Natural alignment, nothing to do */
 	if (alignment <= natural_alignment || alignment == 1)
 		alignment = 0;
+	/* If the offset is compatible with being aligned on the cacheline size
+	 * and this would only result in increasing the alignment, use the
+	 * cacheline size as it is safe and quite likely to be what was in the
+	 * source.
+	 */
+	else if (alignment < cacheline_size &&
+		 cacheline_size % alignment == 0 &&
+		 byte_offset % cacheline_size == 0)
+		alignment = cacheline_size;
 
 	return alignment;
 }
-- 
2.25.1

Re: [PATCH v3 6/6] btf_loader.c: Use cacheline size to infer alignment

[Arnaldo Carvalho de Melo]

Em Thu, Oct 28, 2021 at 01:27:10PM +0100, Douglas RAILLARD escreveu:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> When the alignment is larger than natural, it is very likely that the
> source code was using the cacheline size. Therefore, use the cacheline
> size when it would only result in increasing the alignment.

--- /tmp/btfdiff.dwarf.pXdgRU   2021-10-28 10:22:11.738200232 -0300
+++ /tmp/btfdiff.btf.bkDkdf     2021-10-28 10:22:11.925205061 -0300
@@ -107,7 +107,7 @@ struct Qdisc {
        /* XXX 24 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */
-       struct sk_buff_head        gso_skb __attribute__((__aligned__(64))); /*   128    24 */
+       struct sk_buff_head        gso_skb __attribute__((__aligned__(32))); /*   128    24 */
        struct qdisc_skb_head      q;                    /*   152    24 */
        struct gnet_stats_basic_packed bstats;           /*   176    16 */
        /* --- cacheline 3 boundary (192 bytes) --- */

This one is gone with heuristic, thanks for accepting my suggestion and
coding it this fast!

Applied.

I'm pushing it out to the 'next' branch, please work from there, I'll
move it to 'master' when it passes libbpf's CI tests.

- Arnaldo
 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>  btf_loader.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index e500eae..7a5b16f 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -476,6 +476,7 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
>  				       uint32_t natural_alignment,
>  				       uint32_t smallest_offset)
>  {
> +	uint16_t cacheline_size = conf->conf_fprintf->cacheline_size;
>  	uint32_t alignment = 0;
>  	uint32_t offset_delta = byte_offset - smallest_offset;
>  
> @@ -494,6 +495,15 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
>  	/* Natural alignment, nothing to do */
>  	if (alignment <= natural_alignment || alignment == 1)
>  		alignment = 0;
> +	/* If the offset is compatible with being aligned on the cacheline size
> +	 * and this would only result in increasing the alignment, use the
> +	 * cacheline size as it is safe and quite likely to be what was in the
> +	 * source.
> +	 */
> +	else if (alignment < cacheline_size &&
> +		 cacheline_size % alignment == 0 &&
> +		 byte_offset % cacheline_size == 0)
> +		alignment = cacheline_size;
>  
>  	return alignment;
>  }
> -- 
> 2.25.1

-- 

- Arnaldo

Re: [PATCH v3 6/6] btf_loader.c: Use cacheline size to infer alignment

[Douglas Raillard]

On 10/28/21 2:24 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 28, 2021 at 01:27:10PM +0100, Douglas RAILLARD escreveu:
>> From: Douglas Raillard <douglas.raillard@arm.com>
>>
>> When the alignment is larger than natural, it is very likely that the
>> source code was using the cacheline size. Therefore, use the cacheline
>> size when it would only result in increasing the alignment.
> 
> --- /tmp/btfdiff.dwarf.pXdgRU   2021-10-28 10:22:11.738200232 -0300
> +++ /tmp/btfdiff.btf.bkDkdf     2021-10-28 10:22:11.925205061 -0300
> @@ -107,7 +107,7 @@ struct Qdisc {
>          /* XXX 24 bytes hole, try to pack */
> 
>          /* --- cacheline 2 boundary (128 bytes) --- */
> -       struct sk_buff_head        gso_skb __attribute__((__aligned__(64))); /*   128    24 */
> +       struct sk_buff_head        gso_skb __attribute__((__aligned__(32))); /*   128    24 */
>          struct qdisc_skb_head      q;                    /*   152    24 */
>          struct gnet_stats_basic_packed bstats;           /*   176    16 */
>          /* --- cacheline 3 boundary (192 bytes) --- */
> 
> This one is gone with heuristic, thanks for accepting my suggestion and
> coding it this fast!

Thanks for the quick review !
 From my manual checks, I could basically not find any __aligned__(32) anymore
after this patch as expected. The main difference with dwarf left are missing
__aligned__ when the member was already aligned, but it should be harmless and
there is nothing we can do about that unless BTF is augmented.

> 
> Applied.
> 
> I'm pushing it out to the 'next' branch, please work from there, I'll
> move it to 'master' when it passes libbpf's CI tests.

noted, I rebased on "next", sorry for the repeated patches.

- Douglas

> 
> - Arnaldo
>   
>> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
>> ---
>>   btf_loader.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/btf_loader.c b/btf_loader.c
>> index e500eae..7a5b16f 100644
>> --- a/btf_loader.c
>> +++ b/btf_loader.c
>> @@ -476,6 +476,7 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
>>   				       uint32_t natural_alignment,
>>   				       uint32_t smallest_offset)
>>   {
>> +	uint16_t cacheline_size = conf->conf_fprintf->cacheline_size;
>>   	uint32_t alignment = 0;
>>   	uint32_t offset_delta = byte_offset - smallest_offset;
>>   
>> @@ -494,6 +495,15 @@ static uint32_t class__infer_alignment(const struct conf_load *conf,
>>   	/* Natural alignment, nothing to do */
>>   	if (alignment <= natural_alignment || alignment == 1)
>>   		alignment = 0;
>> +	/* If the offset is compatible with being aligned on the cacheline size
>> +	 * and this would only result in increasing the alignment, use the
>> +	 * cacheline size as it is safe and quite likely to be what was in the
>> +	 * source.
>> +	 */
>> +	else if (alignment < cacheline_size &&
>> +		 cacheline_size % alignment == 0 &&
>> +		 byte_offset % cacheline_size == 0)
>> +		alignment = cacheline_size;
>>   
>>   	return alignment;
>>   }
>> -- 
>> 2.25.1
>