This was happening here:
$ pfunct --compile /home/acme/git/build/v5.1-rc4+/block/partitions/check.o > a.c ; head -29 a.c
struct block_device;
struct (null);
typedef _Bool bool;
struct parsed_partitions {
struct block_device * bdev; /* 0 8 */
char name[32]; /* 8 32 */
struct {
sector_t from; /* 40 8 */
sector_t size; /* 48 8 */
int flags; /* 56 4 */
bool has_info; /* 60 1 */
struct partition_meta_info info; /* 61 101 */
} * parts; /* 40 8 */
int next; /* 48 4 */
int limit; /* 52 4 */
bool access_beyond_eod; /* 56 1 */
/* XXX 7 bytes hole, try to pack */
/* --- cacheline 1 boundary (64 bytes) --- */
char * pp_buf; /* 64 8 */
/* size: 72, cachelines: 2, members: 7 */
/* sum members: 65, holes: 1, sum holes: 7 */
/* last cacheline: 8 bytes */
};
$
I.e. we saw a pointer to a struct, so all we need is a forward
declaration for that function, right? Not if it is defined inline, so
just don't emit the forward declaration if the struct name is NULL.
Oops, the offsets in a struct defined inline and that the member is a
pointer need to have its offset restarted from zero...
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
So that we don't try to compile something that wasn't built with a
CFLAGS or even gcc, like .o made from ASM files.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
With this and running like:
$ find ~/git/build/v5.1-rc4+/fs/ -name "*.o" | grep -v .mod.o | \
while read obj ; do
echo $obj ;
fullcircle $obj ;
done
The vast majority of the kernel single compilation unit objects get
the source code for its function prototypes and types used rebuilt,
recompiled and then the original DWARF can be compared with the one
generated from the regenerated C code.
More work needed, but should be a good start and has already helped to
fix several issues, a reported in the previous csets.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Covering typedefs, unions.
Fixes: 99750f244c ("pfunct: Generate a valid return type for the --compile bodies")
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
We need to dig deeper in some emit cases, but for now lets make this
a bit more robust by checking if the types are unnamed.
Things like:
struct b {
struct {
int a;
};
}
This is well handled in the main fprintf routines, but its interaction
with the emit routines need more work, lets at least remove the segfault
and leave fixing this for good for v1.14.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
/home/acme/git/build/v5.1-rc4+/fs/proc/kcore.o
/tmp/fullcircle.Vnd2oz.c:788:29: error: flexible array member in a struct with no named members
char x[]; /* 0 0 */
Original:
include/linux/mmzone.h, line 109:
/*
* zone->lock and the zone lru_lock are two of the hottest locks in the kernel.
* So add a wild amount of padding here to ensure that they fall into separate
* cachelines. There are very few zone structures in the machine, so space
* consumption is not a concern here.
*/
#if defined(CONFIG_SMP)
struct zone_padding {
char x[0];
} ____cacheline_internodealigned_in_smp;
#define ZONE_PADDING(name) struct zone_padding name;
#else
#define ZONE_PADDING(name)
#endif
B0rken:
struct zone_padding {
char x[]; /* 0 0 */
/* size: 0, cachelines: 0, members: 1 */
} __attribute__((__aligned__(64)));
Fixed:
struct zone_padding {
char x[0]; /* 0 0 */
/* size: 0, cachelines: 0, members: 1 */
} __attribute__((__aligned__(64)));
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Consider:
struct ipc64_perm {
__kernel_key_t key;
__kernel_uid32_t uid;
__kernel_gid32_t gid;
__kernel_uid32_t cuid;
__kernel_gid32_t cgid;
__kernel_mode_t mode;
/* pad if mode_t is u16: */
unsigned char __pad1[4 - sizeof(__kernel_mode_t)];
unsigned short seq;
unsigned short __pad2;
__kernel_ulong_t __unused1;
__kernel_ulong_t __unused2;
};
That is a roundabout way of using __attribute__(__aligned__(4)), but
should work nonetheless.
We were not putting the [0] in that zero sized array which ended up
making gcc complain with:
$ gcc -g -c shm.c
shm.c:199:29: error: flexible array member not at end of struct
unsigned char __pad1[]; /* 24 0 */
^~~~~~
$
Now this works, i.e. generates compilable source code out of the
type tags, be it from BTF or from DWARF, i.e. this is all from the
internal representation of such types, agnostic wrt the original type
format.
So, the full circle:
$ pahole -C ipc64_perm /home/acme/git/build/v5.1-rc4+/ipc/shm.o
struct ipc64_perm {
__kernel_key_t key; /* 0 4 */
__kernel_uid32_t uid; /* 4 4 */
__kernel_gid32_t gid; /* 8 4 */
__kernel_uid32_t cuid; /* 12 4 */
__kernel_gid32_t cgid; /* 16 4 */
__kernel_mode_t mode; /* 20 4 */
unsigned char __pad1[0]; /* 24 0 */
short unsigned int seq; /* 24 2 */
short unsigned int __pad2; /* 26 2 */
/* XXX 4 bytes hole, try to pack */
__kernel_ulong_t __unused1; /* 32 8 */
__kernel_ulong_t __unused2; /* 40 8 */
/* size: 48, cachelines: 1, members: 11 */
/* sum members: 44, holes: 1, sum holes: 4 */
/* last cacheline: 48 bytes */
};
$ pfunct --compile /home/acme/git/build/v5.1-rc4+/ipc/shm.o > shm.c
$ gcc -g -c shm.c
$ pahole -C ipc64_perm shm.o
struct ipc64_perm {
__kernel_key_t key; /* 0 4 */
__kernel_uid32_t uid; /* 4 4 */
__kernel_gid32_t gid; /* 8 4 */
__kernel_uid32_t cuid; /* 12 4 */
__kernel_gid32_t cgid; /* 16 4 */
__kernel_mode_t mode; /* 20 4 */
unsigned char __pad1[0]; /* 24 0 */
short unsigned int seq; /* 24 2 */
short unsigned int __pad2; /* 26 2 */
/* XXX 4 bytes hole, try to pack */
__kernel_ulong_t __unused1; /* 32 8 */
__kernel_ulong_t __unused2; /* 40 8 */
/* size: 48, cachelines: 1, members: 11 */
/* sum members: 44, holes: 1, sum holes: 4 */
/* last cacheline: 48 bytes */
};
$
And for a chuckle, the original source code with a bit of history about
struct layout worries:
include/uapi/asm-generic/ipcbuf.h:
/*
* The generic ipc64_perm structure:
* Note extra padding because this structure is passed back and forth
* between kernel and user space.
*
* ipc64_perm was originally meant to be architecture specific, but
* everyone just ended up making identical copies without specific
* optimizations, so we may just as well all use the same one.
*
* Pad space is left for:
* - 32-bit mode_t on architectures that only had 16 bit
* - 32-bit seq
* - 2 miscellaneous 32-bit values
*/
struct ipc64_perm {
__kernel_key_t key;
__kernel_uid32_t uid;
__kernel_gid32_t gid;
__kernel_uid32_t cuid;
__kernel_gid32_t cgid;
__kernel_mode_t mode;
/* pad if mode_t is u16: */
unsigned char __pad1[4 - sizeof(__kernel_mode_t)];
unsigned short seq;
unsigned short __pad2;
__kernel_ulong_t __unused1;
__kernel_ulong_t __unused2;
};
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Since we don't have something like DW_AT_alignment for
__attribute__((__packed__)), we need to use whatever hints that are
there in the alignments to figure out if a naturally packed struct has
the __attribute__((packed)) in the original sources, because that is
needed to waiver its natural alignment requisites.
For instance,
/* Used at: btrfs.c */
/* <1e7b> /home/acme/git/pahole/btrfs.c:199 */
struct btrfs_block_group_cache {
struct btrfs_key key; /* 0 17 */
struct btrfs_block_group_item item; /* 17 24 */
/* XXX 7 bytes hole, try to pack */
struct btrfs_fs_info * fs_info; /* 48 8 */
struct inode * inode; /* 56 8 */
In the original source code, btrfs_block_group_item is marked
__packed__, and being so, even seemingly unnecessarily, makes it, when
embedded in another struct, like the above, forfeit its natural
alingment, that would be 8 bytes, and instead appear right at the 17th
byte offset...
struct btrfs_block_group_item {
__le64 used; /* 0 8 */
__le64 chunk_objectid; /* 8 8 */
__le64 flags; /* 16 8 */
/* size: 24, cachelines: 1, members: 3 */
/* last cacheline: 24 bytes */
} __attribute__((__packed__));
So we need to, seeing its use at a unnatural offset, go backwards to the
btrfs_block_group_item pahole internal data structure, 'struct type' and
mark is_packed field as 'true', despite it not looking like a packed
struct.
Same thing with:
struct ieee80211_mcs_info {
u8 rx_mask[10]; /* 0 10 */
__le16 rx_highest; /* 10 2 */
u8 tx_params; /* 12 1 */
u8 reserved[3]; /* 13 3 */
/* size: 16, cachelines: 1, members: 4 */
/* last cacheline: 16 bytes */
};
That is naturally aligned and as 16 bytes, a power of two, then when it appears at the end of:
$ pahole -IC ieee80211_sta_ht_cap vht.o
/* Used at: vht.c */
/* <31ea> /home/acme/git/pahole/vht.c:1769 */
struct ieee80211_sta_ht_cap {
u16 cap; /* 0 2 */
bool ht_supported; /* 2 1 */
u8 ampdu_factor; /* 3 1 */
u8 ampdu_density; /* 4 1 */
/* XXX 1 byte hole, try to pack */
struct ieee80211_mcs_info mcs; /* 6 16 */
/* size: 22, cachelines: 1, members: 5 */
/* sum members: 21, holes: 1, sum holes: 1 */
/* last cacheline: 22 bytes */
};
$
We get that one byte hole if ieee80211_mcs_info isn't marked __packed__, as soon as we mark it:
$ pahole -IC ieee80211_sta_ht_cap vht.o
/* Used at: vht.c */
/* <31ea> /home/acme/git/pahole/vht.c:1769 */
struct ieee80211_sta_ht_cap {
u16 cap; /* 0 2 */
bool ht_supported; /* 2 1 */
u8 ampdu_factor; /* 3 1 */
u8 ampdu_density; /* 4 1 */
struct ieee80211_mcs_info mcs; /* 5 16 */
/* size: 22, cachelines: 1, members: 5 */
/* padding: 1 */
/* last cacheline: 22 bytes */
};
[acme@quaco pahole]$
It works, so __packed__ in this case just says: trow away the natural
alignment, make it 1 in whatever container structs.
So, before emitting the types for some struct, we go back looking at
each of its members and checking for such unnatural offsets, marking the
types as __packed__. Now:
$ pfunct --compile /home/acme/git/build/v5.1-rc4+/net/mac80211/vht.o | grep "^struct ieee80211_mcs_info" -A8
struct ieee80211_mcs_info {
u8 rx_mask[10]; /* 0 10 */
__le16 rx_highest; /* 10 2 */
u8 tx_params; /* 12 1 */
u8 reserved[3]; /* 13 3 */
/* size: 16, cachelines: 1, members: 4 */
/* last cacheline: 16 bytes */
} __attribute__((__packed__));
$
$ pfunct --compile /home/acme/git/build/v5.1-rc4+/fs/btrfs/free-space-tree.o | grep "^struct btrfs_block_group_item" -A7
struct btrfs_block_group_item {
__le64 used; /* 0 8 */
__le64 chunk_objectid; /* 8 8 */
__le64 flags; /* 16 8 */
/* size: 24, cachelines: 1, members: 3 */
/* last cacheline: 24 bytes */
} __attribute__((__packed__));
$
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
We use things like DW_AT_alignment, so not all of those attributes are
inferred by formats like BTF that lack that info, allow suppressing the
output and make btfdiff ask for both DWARF and BTF output to have this
suppressed.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
For instance, DWARF has DW_AT_alignment, and some output features
require that, so let loaders advertise such things, next patch will use
this info.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
When the first arg, the old object file has multiple compile units, i.e.
multiple objects that were then linked into one, and the second just one
.o, or equivalent, i.e. a .BTF file, then codiff shouldn't try to
find the types in the single CU in each of the old CUs.
Think about a .BTF file generated from a multi-CU DWARF binary, it will
contain all the types in all of the DWARF CUs, so if we go on trying to
find all the BTF files in each of the CUs, we'll fail.
It only makes sense to go on the DWARF CUs looking for the type on the
.BTF section and then compare them.
Fixes: 6b1e43f2c1 ("codiff: When comparing against a file with just one CU don't bother finding by name")
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Fixes: 13e5b9fc00 ("fprintf: Add unnamed bitfield padding at the end to rebuild original type")
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Like in:
static int
cifs_setlease(struct file *file, long arg, struct file_lock **lease, void **priv)
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Check if the size is different than sizeof(int), which should be good
enough for now for both 64-bit and 32-bit targets.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
I.e. honour conf_fprintf.suppress_aligned_attribute, noticed with
btfdiff, as BTF doesn't carries the alignment attribute, so can't
regenerate it, we need to suppress it so as to compare the output of
DWARF with that of the equivalent BTF.
Fixes: b42d77b0bb ("fprintf: Print __attribute__((__aligned__(N))) for structs/classes")
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
To match the case when we really have just one dimension, so the
--flat-arrays should show for zero sized arrays, [], not [0]:
Noticed with btfdiff.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
We want to reach array__fprintf() from here, with the class_member
name, as __tag__name() isn't handling arrays properly.
I.e. to print an array when we have its name we can't use __tag__name().
This also stops printing 0 for zero sized arrays and trows away the
extra DW_TAG_const_type that comes with zero sized arrays, where we
have:
class_member type: DW_TAG_const_type 1
DW_TAG_const_type 1: DW_TAG_array_type 2
DW_TAG_array_type 2: 0 entries, type: DW_TAG_const_type 3
DW_TAG_const_type 3: real type of the zero sized array
For instance, after this patch we get a sane reconstruction of this
type:
$ pahole -C filename /home/acme/git/build/v5.0-rc2+/ipc/mqueue.o
struct filename {
const char * name; /* 0 8 */
const char * uptr; /* 8 8 */
int refcnt; /* 16 4 */
/* XXX 4 bytes hole, try to pack */
struct audit_names * aname; /* 24 8 */
const char iname[]; /* 32 0 */
/* size: 32, cachelines: 1, members: 5 */
/* sum members: 28, holes: 1, sum holes: 4 */
/* last cacheline: 32 bytes */
};
$
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
I.e. when we find that the last member has a bit_hole, i.e. it is part
of a bitfield, and the current field has a bitfield_size, i.e. it _also_
is part of a bitfield, the only explanation is that they were
artificially put in different base types, i.e. like in these fields
in the linux kernel 'struct task_struct', here reconstructed by pahole:
$ pahole -C task_struct ~/git/build/v5.1-rc2+/kernel/sched/core.o | grep :0 -B9 -A12
unsigned int personality; /* 1128 4 */
unsigned int sched_reset_on_fork:1; /* 1132: 0 4 */
unsigned int sched_contributes_to_load:1; /* 1132: 1 4 */
unsigned int sched_migrated:1; /* 1132: 2 4 */
unsigned int sched_remote_wakeup:1; /* 1132: 3 4 */
/* XXX 28 bits hole, try to pack */
/* Force alignment to the next boundary: */
unsigned int :0;
unsigned int in_execve:1; /* 1136: 0 4 */
unsigned int in_iowait:1; /* 1136: 1 4 */
unsigned int restore_sigmask:1; /* 1136: 2 4 */
unsigned int in_user_fault:1; /* 1136: 3 4 */
unsigned int no_cgroup_migration:1; /* 1136: 4 4 */
unsigned int use_memdelay:1; /* 1136: 5 4 */
/* XXX 26 bits hole, try to pack */
/* XXX 4 bytes hole, try to pack */
long unsigned int atomic_flags; /* 1144 8 */
$
This matches the original definition in the original kernel sources, and
further more, the following sequence proves that with this and DW_AT_alignment,
we can go full circle, i.e.:
1. from an object file reconstruct the source code for all the types that
appears in function signatures, if pointers, them they will be fully defined,
not just forward declared:
$ pfunct --compile=sched_change_group ~/git/build/v5.1-rc2+/kernel/sched/core.o | egrep -w 'sched_change_group|task_struct {' -B10 -A5
/* --- cacheline 3 boundary (192 bytes) --- */
struct fpu fpu __attribute__((__aligned__(64))); /* 192 4160 */
/* size: 4352, cachelines: 68, members: 21 */
/* sum members: 4316, holes: 2, sum holes: 32 */
/* sum bitfield members: 2 bits, bit holes: 1, sum bit holes: 30 bits */
/* forced alignments: 1, forced holes: 1, sum forced holes: 28 */
};
struct task_struct {
struct thread_info thread_info; /* 0 16 */
/* XXX last struct has 4 bytes of padding */
volatile long int state; /* 16 8 */
--
/* --- cacheline 104 boundary (6656 bytes) --- */
struct thread_struct thread __attribute__((__aligned__(64))); /* 6656 4352 */
/* size: 11008, cachelines: 172, members: 207 */
/* sum members: 10902, holes: 16, sum holes: 98 */
/* sum bitfield members: 10 bits, bit holes: 2, sum bit holes: 54 bits */
/* paddings: 3, sum paddings: 14 */
/* forced alignments: 6, forced holes: 1, sum forced holes: 40 */
};
void sched_change_group(struct task_struct * tsk, int type)
{
}
$
2. Build the regenerated skeleton function + its types:
$ pfunct --compile=sched_change_group ~/git/build/v5.1-rc2+/kernel/sched/core.o > sched_change_group.c
$ gcc -g -c sched_change_group.c
$ file sched_change_group.o
sched_change_group.o: ELF 64-bit LSB relocatable, x86-64, version 1 (SYSV), with debug_info, not stripped
$
3. Now lets see if the original 'struct task_struct' printed by pahole, matches
the the output printed by pahole for the DWARF info generated for the regenerated
'struct task_struct' source code in sched_change_group.c:
$ pahole -C task_struct sched_change_group.o | tail
/* --- cacheline 104 boundary (6656 bytes) --- */
struct thread_struct thread __attribute__((__aligned__(64))); /* 6656 4352 */
/* size: 11008, cachelines: 172, members: 207 */
/* sum members: 10902, holes: 16, sum holes: 98 */
/* sum bitfield members: 10 bits, bit holes: 2, sum bit holes: 54 bits */
/* paddings: 3, sum paddings: 14 */
/* forced alignments: 6, forced holes: 1, sum forced holes: 40 */
};
$ pahole -C task_struct ~/git/build/v5.1-rc2+/kernel/sched/core.o | tail
/* --- cacheline 104 boundary (6656 bytes) --- */
struct thread_struct thread __attribute__((__aligned__(64))); /* 6656 4352 */
/* size: 11008, cachelines: 172, members: 207 */
/* sum members: 10902, holes: 16, sum holes: 98 */
/* sum bitfield members: 10 bits, bit holes: 2, sum bit holes: 54 bits */
/* paddings: 3, sum paddings: 14 */
/* forced alignments: 6, forced holes: 1, sum forced holes: 40 */
};
$
Furthermore:
$ pahole -C task_struct ~/git/build/v5.1-rc2+/kernel/sched/core.o > /tmp/original
$ pahole -C task_struct sched_change_group.o > /tmp/regenerated
$ diff -u /tmp/original /tmp/regenerated
$
So one of the most complex data structures in the Linux kernel seems to be under control,
and it uses zero sized unnamed bitfields and __attribute__((aligned(N))), a DWARF5 goodie,
time to go tag v1.13!
Cc: Alexei Starovoitov <ast@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
I.e. 'union {};', 'struct {};' members were always appearing as having
been removed, as we normally do lookup by member name, to find out if
its offset, size, type, etc changed.
For unnamed members, try a different heuristic, i.e. look for the nth
anonymous member, this way we're just trying to compare the first
unnamed member of, say, struct OLD with the first unnamed member of
struct NEW, etc.
For OLD == NEW, this works well, for OLD != NEW because some non
anonymous field got added, removed or moved around, ditto, and when the
number of unnamed fields gets decreased, then we can mix things up, and
compare the previously first in A with the previously first in B.
For the current intended use case of:
1) compile a .c file into a .o file with debugging info, say FILE.o
2) use 'pfunct --compile FILE.o > regenerated-FILE.c'
3) compile regenerated-FILE.c into regenerated-FILE.o with debugging info
4) codiff --struct FILE.o regenerated-FILE.o and find out if they match
This gets us moving forward as we'll spot differences with this algo.
For the future we can use a few more heuristics or stop using search by
name members, instead traversing both structs in tandem, spotting the
differences by comparing the fields that way.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
I.e. when we have two object files with debugging info, and one of them
jas just one CU, then compare all the CUs in the other file to this
unique CU.
Case in hand: encode BTF in a file, then the BTF info has everything in
just one "compile unit", so when looking at the types in the DWARF
originals, we should just compare its types to what is in the single BTF
"compile unit".
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Will be used when considering comparing multiple CU entries in a struct
cus to the sole compile unit in a second file, like when comparing the
types in a multi-CU DWARF file like vmlinux against the combined,
deduplicated entries in that vmlinux .BTF section.
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>