[PATCH 00/16] support __packed struct

[PATCH 00/16] support __packed struct

[Luc Van Oostenryck]

During parsing, Sparse recognizes the attribute 'packed' but this
attribute is otherwise ignored for several reasons:
1) the attribute in 'struct __attr { ... }' is wrongly handled as
   belonging to the whole declaration but it should belong to the type,
2) the attribute in 'struct <name> { ... } __attr;' is simply ignored,
3) the layout of packed bitfields need special care.

This series contains 2 parts:
1) handling of type attributes
2) correct layout of packed structs, including packed bitfields.


This series is also available for review and testing at:
  git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git packed-v2


Changes since v1:
* fix layout of packed bitfields

Luc Van Oostenryck (16):
  add testcases for dubious enum values
  add testcases for exotic enum values
  add testcases for enum attributes
  add testcases for type attributes
  add testcases for packed structures
  add testcases for packed bitfields
  apply_ctype: use self-explanatory argument name
  apply_ctype: reverse the order of arguments
  apply_ctype: move up its declaration
  struct-attr: prepare to handle attributes at the end of struct definitions (1)
  struct-attr: prepare to handle attributes at the end of struct definitions (2)
  struct-attr: prepare to handle attributes at the end of struct definitions (3)
  struct-attr: fix type attribute like 'struct __attr { ... }'
  struct-attr: fix: do not ignore struct/union/enum type attributes
  packed: no out-of-bound access of packed bitfields
  packed: add support for __packed struct

 linearize.c                       | 13 +++++-
 parse.c                           | 71 ++++++++++++++++---------------
 symbol.c                          | 12 ++++--
 symbol.h                          |  2 +
 validation/enum-type-dubious.c    | 18 ++++++++
 validation/enum-type-exotic.c     | 28 ++++++++++++
 validation/packed-bitfield0.c     | 66 ++++++++++++++++++++++++++++
 validation/packed-bitfield1.c     | 27 ++++++++++++
 validation/packed-bitfield2.c     | 15 +++++++
 validation/packed-bitfield3.c     | 28 ++++++++++++
 validation/packed-bitfield4.c     | 18 ++++++++
 validation/packed-bitfield5.c     | 20 +++++++++
 validation/packed-deref0.c        | 23 ++++++++++
 validation/packed-struct.c        | 32 ++++++++++++++
 validation/parsing/enum-attr.c    | 29 +++++++++++++
 validation/type-attribute-align.c | 19 +++++++++
 validation/type-attribute-as.c    | 33 ++++++++++++++
 validation/type-attribute-mod.c   | 21 +++++++++
 validation/type-attribute-qual.c  | 12 ++++++
 19 files changed, 448 insertions(+), 39 deletions(-)
 create mode 100644 validation/enum-type-dubious.c
 create mode 100644 validation/enum-type-exotic.c
 create mode 100644 validation/packed-bitfield0.c
 create mode 100644 validation/packed-bitfield1.c
 create mode 100644 validation/packed-bitfield2.c
 create mode 100644 validation/packed-bitfield3.c
 create mode 100644 validation/packed-bitfield4.c
 create mode 100644 validation/packed-bitfield5.c
 create mode 100644 validation/packed-deref0.c
 create mode 100644 validation/packed-struct.c
 create mode 100644 validation/parsing/enum-attr.c
 create mode 100644 validation/type-attribute-align.c
 create mode 100644 validation/type-attribute-as.c
 create mode 100644 validation/type-attribute-mod.c
 create mode 100644 validation/type-attribute-qual.c


base-commit: 1b896707d95982c7c9cdd5cd0ab4afd80f766a94
Cc: Jacob Keller <jacob.e.keller@intel.com>
-- 
2.29.2

[PATCH v2 01/16] add testcases for dubious enum values

[Luc Van Oostenryck]

sparse accept any type of integral value for enumerators
but address constants are also accepted, which is 'strange'.

Add a testcase for such 'enums'.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/enum-type-dubious.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 validation/enum-type-dubious.c

diff --git a/validation/enum-type-dubious.c b/validation/enum-type-dubious.c
new file mode 100644
index 000000000000..f2cb39fcdbff
--- /dev/null
+++ b/validation/enum-type-dubious.c
@@ -0,0 +1,18 @@
+enum foobar {
+	FOO = (void*)0,
+	BAR = (void*)1,
+	BAZ = (int*)0,
+	QUX = (int*)123,
+};
+
+/*
+ * check-name: enum-type-dubious
+ * check-known-to-fail
+ *
+ * check-error-start
+validation/enum-type-dubious.c:2:8: error: enumerator value for 'FOO' is not an integer constant
+validation/enum-type-dubious.c:3:8: error: enumerator value for 'BAR' is not an integer constant
+validation/enum-type-dubious.c:4:8: error: enumerator value for 'BAZ' is not an integer constant
+validation/enum-type-dubious.c:5:8: error: enumerator value for 'QUX' is not an integer constant
+ * check-error-end
+ */
-- 
2.29.2

[PATCH v2 02/16] add testcases for exotic enum values

[Luc Van Oostenryck]

There is more than one complexity in the evaluation of enums.

Add a test for enums with 'exotic' values non-covered in other tests.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/enum-type-exotic.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 validation/enum-type-exotic.c

diff --git a/validation/enum-type-exotic.c b/validation/enum-type-exotic.c
new file mode 100644
index 000000000000..a17ca0ad48ef
--- /dev/null
+++ b/validation/enum-type-exotic.c
@@ -0,0 +1,28 @@
+enum foobar {
+        C = (unsigned char)0,
+        L = 1L,
+};
+
+unsigned int foo(void);
+unsigned int foo(void)
+{
+#ifdef __CHECKER__
+	_Static_assert([typeof(C)] == [enum foobar], "enum type");
+	_Static_assert([typeof(C)] != [unsigned char], "char type");
+#endif
+
+	typeof(C) v = ~0;
+	return v;
+}
+
+/*
+ * check-name: enum-type-exotic
+ * check-description:
+ *	GCC type's for C is 'int' or maybe 'unsigned int'
+ *	but certainly not 'unsigned char' like here.
+ * check-command: test-linearize -Wno-decl $file
+ * check-known-to-fail
+ *
+ * check-output-ignore
+ * check-output-excludes: ret\\.32 *\\$255
+ */
-- 
2.29.2

Re: [PATCH v2 02/16] add testcases for exotic enum values

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> There is more than one complexity in the evaluation of enums.
> 
> Add a test for enums with 'exotic' values non-covered in other tests.

s/non-covered/not covered/

ATB,
Ramsay Jones

> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  validation/enum-type-exotic.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 validation/enum-type-exotic.c
> 
> diff --git a/validation/enum-type-exotic.c b/validation/enum-type-exotic.c
> new file mode 100644
> index 000000000000..a17ca0ad48ef
> --- /dev/null
> +++ b/validation/enum-type-exotic.c
> @@ -0,0 +1,28 @@
> +enum foobar {
> +        C = (unsigned char)0,
> +        L = 1L,
> +};
> +
> +unsigned int foo(void);
> +unsigned int foo(void)
> +{
> +#ifdef __CHECKER__
> +	_Static_assert([typeof(C)] == [enum foobar], "enum type");
> +	_Static_assert([typeof(C)] != [unsigned char], "char type");
> +#endif
> +
> +	typeof(C) v = ~0;
> +	return v;
> +}
> +
> +/*
> + * check-name: enum-type-exotic
> + * check-description:
> + *	GCC type's for C is 'int' or maybe 'unsigned int'
> + *	but certainly not 'unsigned char' like here.
> + * check-command: test-linearize -Wno-decl $file
> + * check-known-to-fail
> + *
> + * check-output-ignore
> + * check-output-excludes: ret\\.32 *\\$255
> + */
> 

Re: [PATCH v2 02/16] add testcases for exotic enum values

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:10:06PM +0000, Ramsay Jones wrote:
> 
> 
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > There is more than one complexity in the evaluation of enums.
> > 
> > Add a test for enums with 'exotic' values non-covered in other tests.
> 
> s/non-covered/not covered/

Yes, indeed. Thanks.
-- Luc 

[PATCH v2 03/16] add testcases for enum attributes

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/parsing/enum-attr.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 validation/parsing/enum-attr.c

diff --git a/validation/parsing/enum-attr.c b/validation/parsing/enum-attr.c
new file mode 100644
index 000000000000..a962d8b417af
--- /dev/null
+++ b/validation/parsing/enum-attr.c
@@ -0,0 +1,29 @@
+#define __attr __attribute__((deprecated))
+
+enum {
+	old __attr,
+	cur __attr = 42,
+	new,
+};
+
+enum odd {
+	odd = __attr 33,
+};
+
+enum bad {
+	bad = 43 __attr,
+};
+
+/*
+ * check-name: enum-attr
+ *
+ * check-error-start
+parsing/enum-attr.c:10:15: error: typename in expression
+parsing/enum-attr.c:10:15: error: undefined identifier '__attribute__'
+parsing/enum-attr.c:10:15: error: bad constant expression type
+parsing/enum-attr.c:10:22: error: Expected } at end of struct-union-enum-specifier
+parsing/enum-attr.c:10:22: error: got 33
+parsing/enum-attr.c:14:18: error: Expected } at end of struct-union-enum-specifier
+parsing/enum-attr.c:14:18: error: got __attribute__
+ * check-error-end
+ */
-- 
2.29.2

[PATCH v2 04/16] add testcases for type attributes

[Luc Van Oostenryck]

Currently, type attributes are not handled correctly.

Add some testcases for them.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/type-attribute-align.c | 20 ++++++++++++++++++
 validation/type-attribute-as.c    | 34 +++++++++++++++++++++++++++++++
 validation/type-attribute-mod.c   | 22 ++++++++++++++++++++
 validation/type-attribute-qual.c  | 12 +++++++++++
 4 files changed, 88 insertions(+)
 create mode 100644 validation/type-attribute-align.c
 create mode 100644 validation/type-attribute-as.c
 create mode 100644 validation/type-attribute-mod.c
 create mode 100644 validation/type-attribute-qual.c

diff --git a/validation/type-attribute-align.c b/validation/type-attribute-align.c
new file mode 100644
index 000000000000..d9358bff8327
--- /dev/null
+++ b/validation/type-attribute-align.c
@@ -0,0 +1,20 @@
+#define __aligned(N)	__attribute__((aligned(N)))
+#define alignof(X)	__alignof__(X)
+
+struct s {
+	short a, b, c;
+} __aligned(2*sizeof(short));
+
+static int fs(void) { return  sizeof(struct s); }
+static int fa(void) { return alignof(struct s); }
+
+void main(void)
+{
+	_Static_assert( sizeof(struct s) == 4 * sizeof(short), "size");
+	_Static_assert(alignof(struct s) == 2 * sizeof(short), "alignment");
+}
+
+/*
+ * check-name: type-attribute-align
+ * check-known-to-fail
+ */
diff --git a/validation/type-attribute-as.c b/validation/type-attribute-as.c
new file mode 100644
index 000000000000..b40b4e7dddf5
--- /dev/null
+++ b/validation/type-attribute-as.c
@@ -0,0 +1,34 @@
+#define	__as		__attribute__((address_space(__as)))
+
+struct s {
+	int i;
+} __as;
+
+
+extern void use0(void *);
+extern void use1(void __as *);
+
+void main(void)
+{
+	struct s s;
+	int i;
+
+	use0(&s);	// KO
+	use0(&i);	// OK
+	use1(&s);	// OK
+	use1(&i);	// KO
+}
+
+/*
+ * check-name: type-attribute-as
+ * check-known-to-fail
+ *
+ * check-error-start
+type-attribute-as.c:16:15: warning: incorrect type in argument 1 (different address spaces)
+type-attribute-as.c:16:15:    expected void *
+type-attribute-as.c:16:15:    got struct s __as *
+type-attribute-as.c:19:15: warning: incorrect type in argument 1 (different address spaces)
+type-attribute-as.c:19:15:    expected void __as *
+type-attribute-as.c:19:15:    got int *
+ * check-error-end
+ */
diff --git a/validation/type-attribute-mod.c b/validation/type-attribute-mod.c
new file mode 100644
index 000000000000..0e7b166a4aec
--- /dev/null
+++ b/validation/type-attribute-mod.c
@@ -0,0 +1,22 @@
+#define	__noderef	__attribute__((noderef))
+
+struct s {
+	int i;
+} __noderef;
+
+
+void main(void)
+{
+	struct s s;
+
+	s.i = 0;
+}
+
+/*
+ * check-name: type-attribute-mod
+ * check-known-to-fail
+ *
+ * check-error-start
+type-attribute-mod.c:12:9: warning: dereference of noderef expression
+ * check-error-end
+ */
diff --git a/validation/type-attribute-qual.c b/validation/type-attribute-qual.c
new file mode 100644
index 000000000000..ab19a605bda1
--- /dev/null
+++ b/validation/type-attribute-qual.c
@@ -0,0 +1,12 @@
+static const struct s {
+	int x;
+} map[2];
+
+static void foo(struct s *p, int v)
+{
+	p->x += v;
+}
+
+/*
+ * check-name: type-attribute-qual
+ */
-- 
2.29.2

Re: [PATCH v2 04/16] add testcases for type attributes

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> Currently, type attributes are not handled correctly.
> 
> Add some testcases for them.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  validation/type-attribute-align.c | 20 ++++++++++++++++++
>  validation/type-attribute-as.c    | 34 +++++++++++++++++++++++++++++++
>  validation/type-attribute-mod.c   | 22 ++++++++++++++++++++
>  validation/type-attribute-qual.c  | 12 +++++++++++
>  4 files changed, 88 insertions(+)
>  create mode 100644 validation/type-attribute-align.c
>  create mode 100644 validation/type-attribute-as.c
>  create mode 100644 validation/type-attribute-mod.c
>  create mode 100644 validation/type-attribute-qual.c
> 
> diff --git a/validation/type-attribute-align.c b/validation/type-attribute-align.c
> new file mode 100644
> index 000000000000..d9358bff8327
> --- /dev/null
> +++ b/validation/type-attribute-align.c
> @@ -0,0 +1,20 @@
> +#define __aligned(N)	__attribute__((aligned(N)))
> +#define alignof(X)	__alignof__(X)
> +
> +struct s {
> +	short a, b, c;
> +} __aligned(2*sizeof(short));
> +
> +static int fs(void) { return  sizeof(struct s); }
> +static int fa(void) { return alignof(struct s); }
> +
> +void main(void)
> +{
> +	_Static_assert( sizeof(struct s) == 4 * sizeof(short), "size");
> +	_Static_assert(alignof(struct s) == 2 * sizeof(short), "alignment");
> +}
> +
> +/*
> + * check-name: type-attribute-align
> + * check-known-to-fail
> + */
> diff --git a/validation/type-attribute-as.c b/validation/type-attribute-as.c
> new file mode 100644
> index 000000000000..b40b4e7dddf5
> --- /dev/null
> +++ b/validation/type-attribute-as.c
> @@ -0,0 +1,34 @@
> +#define	__as		__attribute__((address_space(__as)))
> +
> +struct s {
> +	int i;
> +} __as;
> +
> +
> +extern void use0(void *);
> +extern void use1(void __as *);
> +
> +void main(void)
> +{
> +	struct s s;
> +	int i;
> +
> +	use0(&s);	// KO
> +	use0(&i);	// OK
> +	use1(&s);	// OK
> +	use1(&i);	// KO
> +}
> +
> +/*
> + * check-name: type-attribute-as
> + * check-known-to-fail
> + *
> + * check-error-start
> +type-attribute-as.c:16:15: warning: incorrect type in argument 1 (different address spaces)
> +type-attribute-as.c:16:15:    expected void *
> +type-attribute-as.c:16:15:    got struct s __as *
> +type-attribute-as.c:19:15: warning: incorrect type in argument 1 (different address spaces)
> +type-attribute-as.c:19:15:    expected void __as *
> +type-attribute-as.c:19:15:    got int *
> + * check-error-end
> + */
> diff --git a/validation/type-attribute-mod.c b/validation/type-attribute-mod.c
> new file mode 100644
> index 000000000000..0e7b166a4aec
> --- /dev/null
> +++ b/validation/type-attribute-mod.c
> @@ -0,0 +1,22 @@
> +#define	__noderef	__attribute__((noderef))
> +
> +struct s {
> +	int i;
> +} __noderef;
> +
> +
> +void main(void)
> +{
> +	struct s s;
> +
> +	s.i = 0;
> +}
> +
> +/*
> + * check-name: type-attribute-mod
> + * check-known-to-fail
> + *
> + * check-error-start
> +type-attribute-mod.c:12:9: warning: dereference of noderef expression
> + * check-error-end
> + */
> diff --git a/validation/type-attribute-qual.c b/validation/type-attribute-qual.c
> new file mode 100644
> index 000000000000..ab19a605bda1
> --- /dev/null
> +++ b/validation/type-attribute-qual.c
> @@ -0,0 +1,12 @@
> +static const struct s {
> +	int x;
> +} map[2];
> +
> +static void foo(struct s *p, int v)
> +{
> +	p->x += v;
> +}

Hmm, I don't understand what this is testing! :(

ATB,
Ramsay Jones

> +
> +/*
> + * check-name: type-attribute-qual
> + */
> 

Re: [PATCH v2 04/16] add testcases for type attributes

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:13:54PM +0000, Ramsay Jones wrote:
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > diff --git a/validation/type-attribute-qual.c b/validation/type-attribute-qual.c
> > new file mode 100644
> > index 000000000000..ab19a605bda1
> > --- /dev/null
> > +++ b/validation/type-attribute-qual.c
> > @@ -0,0 +1,12 @@
> > +static const struct s {
> > +	int x;
> > +} map[2];
> > +
> > +static void foo(struct s *p, int v)
> > +{
> > +	p->x += v;
> > +}
> 
> Hmm, I don't understand what this is testing! :(

The declaration of map also contains the definition of 'struct s'.
The test, by tryin to modify something of type 'struct s', allows to
verify that the 'const' applies only to 'map' and not to 'struct s'.

I'll add a comment.
-- Luc

[PATCH v2 05/16] add testcases for packed structures

[Luc Van Oostenryck]

Currently, packed structs are not handled correctly.

Add some testcases for them.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/packed-deref0.c | 24 ++++++++++++++++++++++++
 validation/packed-struct.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 validation/packed-deref0.c
 create mode 100644 validation/packed-struct.c

diff --git a/validation/packed-deref0.c b/validation/packed-deref0.c
new file mode 100644
index 000000000000..865ad68a4f37
--- /dev/null
+++ b/validation/packed-deref0.c
@@ -0,0 +1,24 @@
+#define	__packed	__attribute__((packed))
+
+typedef struct {
+	__INT8_TYPE__	a;
+	__INT16_TYPE__	b;
+	__INT32_TYPE__	c;
+} __packed obj_t;
+
+_Static_assert(sizeof(obj_t) == 7, "sizeof packed struct");
+
+static void foo(obj_t *ptr, int val)
+{
+	ptr->c = val;
+}
+
+static void bar(obj_t o)
+{
+	foo(&o, 0);
+}
+
+/*
+ * check-name: packed-deref0
+ * check-known-to-fail
+ */
diff --git a/validation/packed-struct.c b/validation/packed-struct.c
new file mode 100644
index 000000000000..5039be4d0b45
--- /dev/null
+++ b/validation/packed-struct.c
@@ -0,0 +1,33 @@
+#define __packed __attribute__((packed))
+
+typedef unsigned char   u8;
+typedef __UINT16_TYPE__ u16;
+typedef __UINT32_TYPE__ u32;
+typedef __UINT64_TYPE__ u64;
+
+struct a {
+	u8 a;
+	u8 b;
+	u16 c;
+} __packed;
+_Static_assert(__alignof(struct a) == 1, "align struct");
+_Static_assert(   sizeof(struct a) == sizeof(u32), " size struct");
+
+struct b {
+	u32	a;
+	u32	b;
+} __packed;
+_Static_assert(__alignof(struct b) == 1, "align struct");
+_Static_assert(   sizeof(struct b) == 8, "size struct");
+
+struct c {
+	u16	a;
+	u32	b;
+} __packed;
+_Static_assert(__alignof(struct c) == 1, "align struct");
+_Static_assert(   sizeof(struct c) == 6, "size struct");
+
+/*
+ * check-name: packed-struct
+ * check-known-to-fail
+ */
-- 
2.29.2

Re: [PATCH v2 05/16] add testcases for packed structures

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> Currently, packed structs are not handled correctly.
> 
> Add some testcases for them.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  validation/packed-deref0.c | 24 ++++++++++++++++++++++++
>  validation/packed-struct.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+)
>  create mode 100644 validation/packed-deref0.c
>  create mode 100644 validation/packed-struct.c
> 
> diff --git a/validation/packed-deref0.c b/validation/packed-deref0.c
> new file mode 100644
> index 000000000000..865ad68a4f37
> --- /dev/null
> +++ b/validation/packed-deref0.c
> @@ -0,0 +1,24 @@
> +#define	__packed	__attribute__((packed))
> +
> +typedef struct {
> +	__INT8_TYPE__	a;
> +	__INT16_TYPE__	b;
> +	__INT32_TYPE__	c;
> +} __packed obj_t;
> +
> +_Static_assert(sizeof(obj_t) == 7, "sizeof packed struct");
> +
> +static void foo(obj_t *ptr, int val)
> +{
> +	ptr->c = val;
> +}
> +
> +static void bar(obj_t o)
> +{
> +	foo(&o, 0);
> +}
> +
> +/*
> + * check-name: packed-deref0
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-struct.c b/validation/packed-struct.c
> new file mode 100644
> index 000000000000..5039be4d0b45
> --- /dev/null
> +++ b/validation/packed-struct.c
> @@ -0,0 +1,33 @@
> +#define __packed __attribute__((packed))
> +
> +typedef unsigned char   u8;
> +typedef __UINT16_TYPE__ u16;
> +typedef __UINT32_TYPE__ u32;
> +typedef __UINT64_TYPE__ u64;
> +
> +struct a {
> +	u8 a;
> +	u8 b;
> +	u16 c;
> +} __packed;
> +_Static_assert(__alignof(struct a) == 1, "align struct");
> +_Static_assert(   sizeof(struct a) == sizeof(u32), " size struct");

Hmm, I don't think '== sizeof(u32)' is any better than '== 4'.

ATB,
Ramsay Jones

> +
> +struct b {
> +	u32	a;
> +	u32	b;
> +} __packed;
> +_Static_assert(__alignof(struct b) == 1, "align struct");
> +_Static_assert(   sizeof(struct b) == 8, "size struct");
> +
> +struct c {
> +	u16	a;
> +	u32	b;
> +} __packed;
> +_Static_assert(__alignof(struct c) == 1, "align struct");
> +_Static_assert(   sizeof(struct c) == 6, "size struct");
> +
> +/*
> + * check-name: packed-struct
> + * check-known-to-fail
> + */
> 

Re: [PATCH v2 05/16] add testcases for packed structures

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:17:36PM +0000, Ramsay Jones wrote:
> 
> 
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > Currently, packed structs are not handled correctly.
> > 
> > Add some testcases for them.
> > 
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  validation/packed-deref0.c | 24 ++++++++++++++++++++++++
> >  validation/packed-struct.c | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+)
> >  create mode 100644 validation/packed-deref0.c
> >  create mode 100644 validation/packed-struct.c
> > 
> > diff --git a/validation/packed-deref0.c b/validation/packed-deref0.c
> > new file mode 100644
> > index 000000000000..865ad68a4f37
> > --- /dev/null
> > +++ b/validation/packed-deref0.c
> > @@ -0,0 +1,24 @@
> > +#define	__packed	__attribute__((packed))
> > +
> > +typedef struct {
> > +	__INT8_TYPE__	a;
> > +	__INT16_TYPE__	b;
> > +	__INT32_TYPE__	c;
> > +} __packed obj_t;
> > +
> > +_Static_assert(sizeof(obj_t) == 7, "sizeof packed struct");
> > +
> > +static void foo(obj_t *ptr, int val)
> > +{
> > +	ptr->c = val;
> > +}
> > +
> > +static void bar(obj_t o)
> > +{
> > +	foo(&o, 0);
> > +}
> > +
> > +/*
> > + * check-name: packed-deref0
> > + * check-known-to-fail
> > + */
> > diff --git a/validation/packed-struct.c b/validation/packed-struct.c
> > new file mode 100644
> > index 000000000000..5039be4d0b45
> > --- /dev/null
> > +++ b/validation/packed-struct.c
> > @@ -0,0 +1,33 @@
> > +#define __packed __attribute__((packed))
> > +
> > +typedef unsigned char   u8;
> > +typedef __UINT16_TYPE__ u16;
> > +typedef __UINT32_TYPE__ u32;
> > +typedef __UINT64_TYPE__ u64;
> > +
> > +struct a {
> > +	u8 a;
> > +	u8 b;
> > +	u16 c;
> > +} __packed;
> > +_Static_assert(__alignof(struct a) == 1, "align struct");
> > +_Static_assert(   sizeof(struct a) == sizeof(u32), " size struct");
> 
> Hmm, I don't think '== sizeof(u32)' is any better than '== 4'.

Yes, I agree.

-- Luc 

[PATCH v2 06/16] add testcases for packed bitfields

[Luc Van Oostenryck]

Currently, packed bitfields are not handled correctly.

Add some testcases for them.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 validation/packed-bitfield0.c | 67 +++++++++++++++++++++++++++++++++++
 validation/packed-bitfield1.c | 28 +++++++++++++++
 validation/packed-bitfield2.c | 16 +++++++++
 validation/packed-bitfield3.c | 29 +++++++++++++++
 validation/packed-bitfield4.c | 19 ++++++++++
 validation/packed-bitfield5.c | 21 +++++++++++
 6 files changed, 180 insertions(+)
 create mode 100644 validation/packed-bitfield0.c
 create mode 100644 validation/packed-bitfield1.c
 create mode 100644 validation/packed-bitfield2.c
 create mode 100644 validation/packed-bitfield3.c
 create mode 100644 validation/packed-bitfield4.c
 create mode 100644 validation/packed-bitfield5.c

diff --git a/validation/packed-bitfield0.c b/validation/packed-bitfield0.c
new file mode 100644
index 000000000000..907500dedbf0
--- /dev/null
+++ b/validation/packed-bitfield0.c
@@ -0,0 +1,67 @@
+#define alignof(X)	__alignof__(X)
+#define __packed	__attribute__((packed))
+
+struct sa {
+	int a:7;
+	int c:10;
+	int b:2;
+} __packed;
+_Static_assert(alignof(struct sa) == 1, "alignof(struct sa)");
+_Static_assert( sizeof(struct sa) == 3,  "sizeof(struct sa)");
+
+struct __packed sb {
+	int a:7;
+	int c:10;
+	int b:2;
+};
+_Static_assert(alignof(struct sb) == 1, "alignof(struct sb)");
+_Static_assert( sizeof(struct sb) == 3,  "sizeof(struct sb)");
+
+
+static int get_size(void)
+{
+	return sizeof(struct sa);
+}
+
+static void chk_align(struct sa sa, struct sa *p)
+{
+	_Static_assert(alignof(sa) == 1, "alignof(sa)");
+	_Static_assert(alignof(*p) == 1, "alignof(*p)");
+}
+
+static int fp0(struct sa *sa)
+{
+	return sa->c;
+}
+
+static int fpx(struct sa *sa, int idx)
+{
+	return sa[idx].c;
+}
+
+static int fglobal(void)
+{
+	extern struct sa g;
+	return g.c;
+}
+
+static struct sa l;
+static int flocal(void)
+{
+	return l.c;
+}
+
+
+int main(void)
+{
+	extern void fun(struct sa *);
+	struct sa sa = { 0 };
+
+	fun(&sa);
+	return 0;
+}
+
+/*
+ * check-name: packed-bitfield0
+ * check-known-to-fail
+ */
diff --git a/validation/packed-bitfield1.c b/validation/packed-bitfield1.c
new file mode 100644
index 000000000000..208a3dc5127c
--- /dev/null
+++ b/validation/packed-bitfield1.c
@@ -0,0 +1,28 @@
+#define __packed	__attribute__((packed))
+
+struct s {
+	unsigned int f0:1;
+	unsigned int f1:1;
+	unsigned int pad:6;
+} __packed;
+_Static_assert(sizeof(struct s) == 1,  "sizeof(struct s)");
+
+extern struct s g;
+
+static int foo(struct s *ptr)
+{
+	int f = 0;
+
+	f += g.f0;
+	f += g.f1;
+
+	f += ptr->f0;
+	f += ptr->f1;
+
+	return f;
+}
+
+/*
+ * check-name: packed-bitfield1
+ * check-known-to-fail
+ */
diff --git a/validation/packed-bitfield2.c b/validation/packed-bitfield2.c
new file mode 100644
index 000000000000..4587ebec5c90
--- /dev/null
+++ b/validation/packed-bitfield2.c
@@ -0,0 +1,16 @@
+struct bf2 {
+	unsigned p1:2;
+	unsigned i1:32;
+	unsigned p2:2;
+	unsigned s9:9;
+	unsigned s9:9;
+	unsigned s9:9;
+	unsigned b1:1;
+} __attribute__((packed));
+
+_Static_assert(sizeof(struct bf2) == 8);
+
+/*
+ * check-name: packed-bitfield2
+ * check-known-to-fail
+ */
diff --git a/validation/packed-bitfield3.c b/validation/packed-bitfield3.c
new file mode 100644
index 000000000000..6acff875299f
--- /dev/null
+++ b/validation/packed-bitfield3.c
@@ -0,0 +1,29 @@
+#define __packed __attribute__((packed))
+
+typedef unsigned char   u8;
+typedef __UINT16_TYPE__ u16;
+typedef __UINT32_TYPE__ u32;
+typedef __UINT64_TYPE__ u64;
+
+struct b {
+	u32	a:1;
+	u32	b:2;
+	u32	c:4;
+	u32	d:8;
+	u32	e:16;
+} __packed;
+_Static_assert(__alignof(struct b) == 1);
+_Static_assert(   sizeof(struct b) == sizeof(u32));
+
+struct c {
+	u8	a;
+	u8	b;
+	u64	c:48;
+} __packed;
+_Static_assert(__alignof(struct c) == 1);
+_Static_assert(   sizeof(struct c) == sizeof(u64));
+
+/*
+ * check-name: packed-bitfield3
+ * check-known-to-fail
+ */
diff --git a/validation/packed-bitfield4.c b/validation/packed-bitfield4.c
new file mode 100644
index 000000000000..0342b2414b6e
--- /dev/null
+++ b/validation/packed-bitfield4.c
@@ -0,0 +1,19 @@
+#define __packed __attribute__((packed))
+
+typedef __UINT32_TYPE__ u32;
+
+struct s {
+	u32	f:24;
+} __packed;
+_Static_assert(sizeof(struct s) == 3);
+
+static int ld(struct s *s)
+{
+	return s->f;
+}
+
+/*
+ * check-name: packed-bitfield4
+ * check-description: Is check_access() OK with short packed bitfields?
+ * check-known-to-fail
+ */
diff --git a/validation/packed-bitfield5.c b/validation/packed-bitfield5.c
new file mode 100644
index 000000000000..8f44d4c2c277
--- /dev/null
+++ b/validation/packed-bitfield5.c
@@ -0,0 +1,21 @@
+#define __packed __attribute__((packed))
+
+typedef __UINT32_TYPE__ u32;
+
+struct s {
+	u32	a:5;
+	u32	f:30;
+	u32	z:5;
+} __packed;
+_Static_assert(sizeof(struct s) == 5);
+
+static int ld(struct s *s)
+{
+	return s->f;
+}
+
+/*
+ * check-name: packed-bitfield5
+ * check-description: is check_access() OK with 'overlapping' packed bitfields?
+ * check-known-to-fail
+ */
-- 
2.29.2

Re: [PATCH v2 06/16] add testcases for packed bitfields

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> Currently, packed bitfields are not handled correctly.
> 
> Add some testcases for them.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  validation/packed-bitfield0.c | 67 +++++++++++++++++++++++++++++++++++
>  validation/packed-bitfield1.c | 28 +++++++++++++++
>  validation/packed-bitfield2.c | 16 +++++++++
>  validation/packed-bitfield3.c | 29 +++++++++++++++
>  validation/packed-bitfield4.c | 19 ++++++++++
>  validation/packed-bitfield5.c | 21 +++++++++++
>  6 files changed, 180 insertions(+)
>  create mode 100644 validation/packed-bitfield0.c
>  create mode 100644 validation/packed-bitfield1.c
>  create mode 100644 validation/packed-bitfield2.c
>  create mode 100644 validation/packed-bitfield3.c
>  create mode 100644 validation/packed-bitfield4.c
>  create mode 100644 validation/packed-bitfield5.c
> 
> diff --git a/validation/packed-bitfield0.c b/validation/packed-bitfield0.c
> new file mode 100644
> index 000000000000..907500dedbf0
> --- /dev/null
> +++ b/validation/packed-bitfield0.c
> @@ -0,0 +1,67 @@
> +#define alignof(X)	__alignof__(X)
> +#define __packed	__attribute__((packed))
> +
> +struct sa {
> +	int a:7;
> +	int c:10;
> +	int b:2;
> +} __packed;
> +_Static_assert(alignof(struct sa) == 1, "alignof(struct sa)");
> +_Static_assert( sizeof(struct sa) == 3,  "sizeof(struct sa)");
> +
> +struct __packed sb {
> +	int a:7;
> +	int c:10;
> +	int b:2;
> +};
> +_Static_assert(alignof(struct sb) == 1, "alignof(struct sb)");
> +_Static_assert( sizeof(struct sb) == 3,  "sizeof(struct sb)");

Why 'struct sb'? It is not used in the rest of the test (and is
identical to 'struct sa').

> +
> +
> +static int get_size(void)
> +{
> +	return sizeof(struct sa);
> +}
> +
> +static void chk_align(struct sa sa, struct sa *p)
> +{
> +	_Static_assert(alignof(sa) == 1, "alignof(sa)");
> +	_Static_assert(alignof(*p) == 1, "alignof(*p)");
> +}
> +
> +static int fp0(struct sa *sa)
> +{
> +	return sa->c;
> +}
> +
> +static int fpx(struct sa *sa, int idx)
> +{
> +	return sa[idx].c;
> +}
> +
> +static int fglobal(void)
> +{
> +	extern struct sa g;
> +	return g.c;
> +}
> +
> +static struct sa l;
> +static int flocal(void)
> +{
> +	return l.c;
> +}
> +
> +
> +int main(void)
> +{
> +	extern void fun(struct sa *);
> +	struct sa sa = { 0 };
> +
> +	fun(&sa);
> +	return 0;
> +}
> +
> +/*
> + * check-name: packed-bitfield0
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-bitfield1.c b/validation/packed-bitfield1.c
> new file mode 100644
> index 000000000000..208a3dc5127c
> --- /dev/null
> +++ b/validation/packed-bitfield1.c
> @@ -0,0 +1,28 @@
> +#define __packed	__attribute__((packed))
> +
> +struct s {
> +	unsigned int f0:1;
> +	unsigned int f1:1;
> +	unsigned int pad:6;
> +} __packed;
> +_Static_assert(sizeof(struct s) == 1,  "sizeof(struct s)");
> +
> +extern struct s g;
> +
> +static int foo(struct s *ptr)
> +{
> +	int f = 0;
> +
> +	f += g.f0;
> +	f += g.f1;
> +
> +	f += ptr->f0;
> +	f += ptr->f1;
> +
> +	return f;
> +}
> +
> +/*
> + * check-name: packed-bitfield1
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-bitfield2.c b/validation/packed-bitfield2.c
> new file mode 100644
> index 000000000000..4587ebec5c90
> --- /dev/null
> +++ b/validation/packed-bitfield2.c
> @@ -0,0 +1,16 @@
> +struct bf2 {
> +	unsigned p1:2;
> +	unsigned i1:32;
> +	unsigned p2:2;
> +	unsigned s9:9;
> +	unsigned s9:9;
> +	unsigned s9:9;
> +	unsigned b1:1;
> +} __attribute__((packed));
> +
> +_Static_assert(sizeof(struct bf2) == 8);
> +
> +/*
> + * check-name: packed-bitfield2
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-bitfield3.c b/validation/packed-bitfield3.c
> new file mode 100644
> index 000000000000..6acff875299f
> --- /dev/null
> +++ b/validation/packed-bitfield3.c
> @@ -0,0 +1,29 @@
> +#define __packed __attribute__((packed))
> +
> +typedef unsigned char   u8;
> +typedef __UINT16_TYPE__ u16;
> +typedef __UINT32_TYPE__ u32;
> +typedef __UINT64_TYPE__ u64;
> +
> +struct b {
> +	u32	a:1;
> +	u32	b:2;
> +	u32	c:4;
> +	u32	d:8;
> +	u32	e:16;
> +} __packed;
> +_Static_assert(__alignof(struct b) == 1);
> +_Static_assert(   sizeof(struct b) == sizeof(u32));

Again '== sizeof(u32)' does not seem useful. (what is it
trying to say?)

> +
> +struct c {
> +	u8	a;
> +	u8	b;
> +	u64	c:48;
> +} __packed;
> +_Static_assert(__alignof(struct c) == 1);
> +_Static_assert(   sizeof(struct c) == sizeof(u64));

ditto.

ATB,
Ramsay Jones

> +
> +/*
> + * check-name: packed-bitfield3
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-bitfield4.c b/validation/packed-bitfield4.c
> new file mode 100644
> index 000000000000..0342b2414b6e
> --- /dev/null
> +++ b/validation/packed-bitfield4.c
> @@ -0,0 +1,19 @@
> +#define __packed __attribute__((packed))
> +
> +typedef __UINT32_TYPE__ u32;
> +
> +struct s {
> +	u32	f:24;
> +} __packed;
> +_Static_assert(sizeof(struct s) == 3);
> +
> +static int ld(struct s *s)
> +{
> +	return s->f;
> +}
> +
> +/*
> + * check-name: packed-bitfield4
> + * check-description: Is check_access() OK with short packed bitfields?
> + * check-known-to-fail
> + */
> diff --git a/validation/packed-bitfield5.c b/validation/packed-bitfield5.c
> new file mode 100644
> index 000000000000..8f44d4c2c277
> --- /dev/null
> +++ b/validation/packed-bitfield5.c
> @@ -0,0 +1,21 @@
> +#define __packed __attribute__((packed))
> +
> +typedef __UINT32_TYPE__ u32;
> +
> +struct s {
> +	u32	a:5;
> +	u32	f:30;
> +	u32	z:5;
> +} __packed;
> +_Static_assert(sizeof(struct s) == 5);
> +
> +static int ld(struct s *s)
> +{
> +	return s->f;
> +}
> +
> +/*
> + * check-name: packed-bitfield5
> + * check-description: is check_access() OK with 'overlapping' packed bitfields?
> + * check-known-to-fail
> + */
> 

Re: [PATCH v2 06/16] add testcases for packed bitfields

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:28:24PM +0000, Ramsay Jones wrote:
> 
> 
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > Currently, packed bitfields are not handled correctly.
> > 
> > Add some testcases for them.
> > 
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  validation/packed-bitfield0.c | 67 +++++++++++++++++++++++++++++++++++
> >  validation/packed-bitfield1.c | 28 +++++++++++++++
> >  validation/packed-bitfield2.c | 16 +++++++++
> >  validation/packed-bitfield3.c | 29 +++++++++++++++
> >  validation/packed-bitfield4.c | 19 ++++++++++
> >  validation/packed-bitfield5.c | 21 +++++++++++
> >  6 files changed, 180 insertions(+)
> >  create mode 100644 validation/packed-bitfield0.c
> >  create mode 100644 validation/packed-bitfield1.c
> >  create mode 100644 validation/packed-bitfield2.c
> >  create mode 100644 validation/packed-bitfield3.c
> >  create mode 100644 validation/packed-bitfield4.c
> >  create mode 100644 validation/packed-bitfield5.c
> > 
> > diff --git a/validation/packed-bitfield0.c b/validation/packed-bitfield0.c
> > new file mode 100644
> > index 000000000000..907500dedbf0
> > --- /dev/null
> > +++ b/validation/packed-bitfield0.c
> > @@ -0,0 +1,67 @@
> > +#define alignof(X)	__alignof__(X)
> > +#define __packed	__attribute__((packed))
> > +
> > +struct sa {
> > +	int a:7;
> > +	int c:10;
> > +	int b:2;
> > +} __packed;
> > +_Static_assert(alignof(struct sa) == 1, "alignof(struct sa)");
> > +_Static_assert( sizeof(struct sa) == 3,  "sizeof(struct sa)");
> > +
> > +struct __packed sb {
> > +	int a:7;
> > +	int c:10;
> > +	int b:2;
> > +};
> > +_Static_assert(alignof(struct sb) == 1, "alignof(struct sb)");
> > +_Static_assert( sizeof(struct sb) == 3,  "sizeof(struct sb)");
> 
> Why 'struct sb'? It is not used in the rest of the test (and is
> identical to 'struct sa').

Good question :)
I've probably reused some previous file as a kind of template.
 
> > diff --git a/validation/packed-bitfield3.c b/validation/packed-bitfield3.c
> > new file mode 100644
> > index 000000000000..6acff875299f
> > --- /dev/null
> > +++ b/validation/packed-bitfield3.c
> > @@ -0,0 +1,29 @@
> > +#define __packed __attribute__((packed))
> > +
> > +typedef unsigned char   u8;
> > +typedef __UINT16_TYPE__ u16;
> > +typedef __UINT32_TYPE__ u32;
> > +typedef __UINT64_TYPE__ u64;
> > +
> > +struct b {
> > +	u32	a:1;
> > +	u32	b:2;
> > +	u32	c:4;
> > +	u32	d:8;
> > +	u32	e:16;
> > +} __packed;
> > +_Static_assert(__alignof(struct b) == 1);
> > +_Static_assert(   sizeof(struct b) == sizeof(u32));
> 
> Again '== sizeof(u32)' does not seem useful. (what is it
> trying to say?)
> 
> > +
> > +struct c {
> > +	u8	a;
> > +	u8	b;
> > +	u64	c:48;
> > +} __packed;
> > +_Static_assert(__alignof(struct c) == 1);
> > +_Static_assert(   sizeof(struct c) == sizeof(u64));
> 
> ditto.

Yes, I agree.

-- Luc 

[PATCH v2 07/16] apply_ctype: use self-explanatory argument name

[Luc Van Oostenryck]

apply_ctype() is used to copy/apply things like modifiers
and address space from one type to another one.
But the names used for the two types are: 'ctype' & 'thistype'
which doesn't help at all to know which one is the 'source' type
and which one is the 'destination' type.

Change this by renaming these arguments to 'src' & 'dst'.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/parse.c b/parse.c
index 0b2685707b82..402214212d77 100644
--- a/parse.c
+++ b/parse.c
@@ -1043,7 +1043,7 @@ static struct token *enum_specifier(struct token *token, struct symbol *sym, str
 	return ret;
 }
 
-static void apply_ctype(struct position pos, struct ctype *thistype, struct ctype *ctype);
+static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst);
 
 static struct token *typeof_specifier(struct token *token, struct symbol *sym, struct decl_state *ctx)
 {
@@ -1427,24 +1427,24 @@ static struct token *generic_qualifier(struct token *next, struct symbol *sym, s
 	return next;
 }
 
-static void apply_ctype(struct position pos, struct ctype *thistype, struct ctype *ctype)
+static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst)
 {
-	unsigned long mod = thistype->modifiers;
+	unsigned long mod = src->modifiers;
 
 	if (mod)
-		apply_qualifier(&pos, ctype, mod);
+		apply_qualifier(&pos, dst, mod);
 
 	/* Context */
-	concat_ptr_list((struct ptr_list *)thistype->contexts,
-	                (struct ptr_list **)&ctype->contexts);
+	concat_ptr_list((struct ptr_list *)src->contexts,
+	                (struct ptr_list **)&dst->contexts);
 
 	/* Alignment */
-	if (thistype->alignment > ctype->alignment)
-		ctype->alignment = thistype->alignment;
+	if (src->alignment > dst->alignment)
+		dst->alignment = src->alignment;
 
 	/* Address space */
-	if (thistype->as)
-		ctype->as = thistype->as;
+	if (src->as)
+		dst->as = src->as;
 }
 
 static void specifier_conflict(struct position pos, int what, struct ident *new)
-- 
2.29.2

[PATCH v2 08/16] apply_ctype: reverse the order of arguments

[Luc Van Oostenryck]

apply_ctype()'s argument order is: src, dst (so the reading
direction) but the assignment/memcpy() order is much more used:
	dst = src;
	memcpy(dst, src, n);
than the order here is confusing.

So, change its argument order to comply with the memcpy()/
assignement order and stop the confusion.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/parse.c b/parse.c
index 402214212d77..f106444f75d8 100644
--- a/parse.c
+++ b/parse.c
@@ -1043,7 +1043,7 @@ static struct token *enum_specifier(struct token *token, struct symbol *sym, str
 	return ret;
 }
 
-static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst);
+static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src);
 
 static struct token *typeof_specifier(struct token *token, struct symbol *sym, struct decl_state *ctx)
 {
@@ -1056,7 +1056,7 @@ static struct token *typeof_specifier(struct token *token, struct symbol *sym, s
 		struct symbol *sym;
 		token = typename(token->next, &sym, NULL);
 		ctx->ctype.base_type = sym->ctype.base_type;
-		apply_ctype(token->pos, &sym->ctype, &ctx->ctype);
+		apply_ctype(token->pos, &ctx->ctype, &sym->ctype);
 	} else {
 		struct symbol *typeof_sym = alloc_symbol(token->pos, SYM_TYPEOF);
 		token = parse_expression(token->next, &typeof_sym->initializer);
@@ -1427,7 +1427,7 @@ static struct token *generic_qualifier(struct token *next, struct symbol *sym, s
 	return next;
 }
 
-static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst)
+static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src)
 {
 	unsigned long mod = src->modifiers;
 
@@ -1529,7 +1529,7 @@ static struct token *declaration_specifiers(struct token *token, struct decl_sta
 				break;
 			seen |= Set_S | Set_T;
 			ctx->ctype.base_type = s->ctype.base_type;
-			apply_ctype(token->pos, &s->ctype, &ctx->ctype);
+			apply_ctype(token->pos, &ctx->ctype, &s->ctype);
 			token = token->next;
 			continue;
 		}
-- 
2.29.2

Re: [PATCH v2 08/16] apply_ctype: reverse the order of arguments

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> apply_ctype()'s argument order is: src, dst (so the reading
> direction) but the assignment/memcpy() order is much more used:
> 	dst = src;
> 	memcpy(dst, src, n);
> than the order here is confusing.
> 
> So, change its argument order to comply with the memcpy()/
> assignement order and stop the confusion.

Hmm, how about:

"""
apply_ctype()'s argument order is 'src' then 'dst', which reads as
copying 'src' to 'dst'. However, assignment, and many library functions
(eg. memcpy()), use the opposite order for the source and destination
of a copy operation.

So, change the argument order of apply_ctype() to mimic the order of
memcpy()/assignment, to hopefully reduce any potential confusion.
"""

Heh, well that is probably not much better! ;-)

However, what about the (er...) position of the 'pos' argument?
Should it move to the end?

ATB,
Ramsay Jones

> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  parse.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/parse.c b/parse.c
> index 402214212d77..f106444f75d8 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -1043,7 +1043,7 @@ static struct token *enum_specifier(struct token *token, struct symbol *sym, str
>  	return ret;
>  }
>  
> -static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst);
> +static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src);
>  
>  static struct token *typeof_specifier(struct token *token, struct symbol *sym, struct decl_state *ctx)
>  {
> @@ -1056,7 +1056,7 @@ static struct token *typeof_specifier(struct token *token, struct symbol *sym, s
>  		struct symbol *sym;
>  		token = typename(token->next, &sym, NULL);
>  		ctx->ctype.base_type = sym->ctype.base_type;
> -		apply_ctype(token->pos, &sym->ctype, &ctx->ctype);
> +		apply_ctype(token->pos, &ctx->ctype, &sym->ctype);
>  	} else {
>  		struct symbol *typeof_sym = alloc_symbol(token->pos, SYM_TYPEOF);
>  		token = parse_expression(token->next, &typeof_sym->initializer);
> @@ -1427,7 +1427,7 @@ static struct token *generic_qualifier(struct token *next, struct symbol *sym, s
>  	return next;
>  }
>  
> -static void apply_ctype(struct position pos, struct ctype *src, struct ctype *dst)
> +static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src)
>  {
>  	unsigned long mod = src->modifiers;
>  
> @@ -1529,7 +1529,7 @@ static struct token *declaration_specifiers(struct token *token, struct decl_sta
>  				break;
>  			seen |= Set_S | Set_T;
>  			ctx->ctype.base_type = s->ctype.base_type;
> -			apply_ctype(token->pos, &s->ctype, &ctx->ctype);
> +			apply_ctype(token->pos, &ctx->ctype, &s->ctype);
>  			token = token->next;
>  			continue;
>  		}
> 

Re: [PATCH v2 08/16] apply_ctype: reverse the order of arguments

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:47:01PM +0000, Ramsay Jones wrote:
> 
> 
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > apply_ctype()'s argument order is: src, dst (so the reading
> > direction) but the assignment/memcpy() order is much more used:
> > 	dst = src;
> > 	memcpy(dst, src, n);
> > than the order here is confusing.
> > 
> > So, change its argument order to comply with the memcpy()/
> > assignement order and stop the confusion.
> 
> Hmm, how about:
> 
> """
> apply_ctype()'s argument order is 'src' then 'dst', which reads as
> copying 'src' to 'dst'. However, assignment, and many library functions
> (eg. memcpy()), use the opposite order for the source and destination
> of a copy operation.
> 
> So, change the argument order of apply_ctype() to mimic the order of
> memcpy()/assignment, to hopefully reduce any potential confusion.
> """
> 
> Heh, well that is probably not much better! ;-)

It's better but I'll try to reformulate it to better express that:
*) the 'reading direction' (left to right) vs. the 'assignment direction'
   (right to left) is a question of API choice
*) the left-to-right direction confuses *me* endlessly, but it's just me.

-- Luc

[PATCH v2 09/16] apply_ctype: move up its declaration

[Luc Van Oostenryck]

apply_ctype() will be needed earlier in the code.
So, move it's prototype up.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/parse.c b/parse.c
index f106444f75d8..d6343f0e48bf 100644
--- a/parse.c
+++ b/parse.c
@@ -669,6 +669,8 @@ struct statement *alloc_statement(struct position pos, int type)
 
 static struct token *struct_declaration_list(struct token *token, struct symbol_list **list);
 
+static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src);
+
 static void apply_modifiers(struct position pos, struct decl_state *ctx)
 {
 	struct symbol *ctype;
@@ -1043,8 +1045,6 @@ static struct token *enum_specifier(struct token *token, struct symbol *sym, str
 	return ret;
 }
 
-static void apply_ctype(struct position pos, struct ctype *dst, struct ctype *src);
-
 static struct token *typeof_specifier(struct token *token, struct symbol *sym, struct decl_state *ctx)
 {
 
-- 
2.29.2

[PATCH v2 10/16] struct-attr: prepare to handle attributes at the end of struct definitions (1)

[Luc Van Oostenryck]

Type attributes for struct can be placed either just after the
keyword 'struct' or after the '}' ending its definition but this
later case is currently ignored.

Prepare the handling of this by factoring the code common to both
cases in a single place.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                        | 11 +++++------
 validation/parsing/enum-attr.c |  4 ++--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/parse.c b/parse.c
index d6343f0e48bf..99d810910dab 100644
--- a/parse.c
+++ b/parse.c
@@ -745,12 +745,7 @@ static struct token *struct_union_enum_specifier(enum type type,
 			if (sym->symbol_list)
 				error_die(token->pos, "redefinition of %s", show_typename (sym));
 			sym->pos = *repos;
-			token = parse(token->next, sym);
-			token = expect(token, '}', "at end of struct-union-enum-specifier");
-
-			// Mark the structure as needing re-examination
-			sym->examined = 0;
-			sym->endpos = token->pos;
+			goto end;
 		}
 		return token;
 	}
@@ -763,10 +758,14 @@ static struct token *struct_union_enum_specifier(enum type type,
 	}
 
 	sym = alloc_symbol(token->pos, type);
+end:
 	set_current_scope(sym);		// used by dissect
 	token = parse(token->next, sym);
 	ctx->ctype.base_type = sym;
 	token =  expect(token, '}', "at end of specifier");
+
+	// Mark the structure as needing re-examination
+	sym->examined = 0;
 	sym->endpos = token->pos;
 
 	return token;
diff --git a/validation/parsing/enum-attr.c b/validation/parsing/enum-attr.c
index a962d8b417af..8d851a162135 100644
--- a/validation/parsing/enum-attr.c
+++ b/validation/parsing/enum-attr.c
@@ -21,9 +21,9 @@ enum bad {
 parsing/enum-attr.c:10:15: error: typename in expression
 parsing/enum-attr.c:10:15: error: undefined identifier '__attribute__'
 parsing/enum-attr.c:10:15: error: bad constant expression type
-parsing/enum-attr.c:10:22: error: Expected } at end of struct-union-enum-specifier
+parsing/enum-attr.c:10:22: error: Expected } at end of specifier
 parsing/enum-attr.c:10:22: error: got 33
-parsing/enum-attr.c:14:18: error: Expected } at end of struct-union-enum-specifier
+parsing/enum-attr.c:14:18: error: Expected } at end of specifier
 parsing/enum-attr.c:14:18: error: got __attribute__
  * check-error-end
  */
-- 
2.29.2

Re: [PATCH v2 10/16] struct-attr: prepare to handle attributes at the end of struct definitions (1)

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> Type attributes for struct can be placed either just after the
> keyword 'struct' or after the '}' ending its definition but this
> later case is currently ignored.
> 
> Prepare the handling of this by factoring the code common to both
> cases in a single place.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  parse.c                        | 11 +++++------
>  validation/parsing/enum-attr.c |  4 ++--
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/parse.c b/parse.c
> index d6343f0e48bf..99d810910dab 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -745,12 +745,7 @@ static struct token *struct_union_enum_specifier(enum type type,
>  			if (sym->symbol_list)
>  				error_die(token->pos, "redefinition of %s", show_typename (sym));
>  			sym->pos = *repos;
> -			token = parse(token->next, sym);
> -			token = expect(token, '}', "at end of struct-union-enum-specifier");
> -
> -			// Mark the structure as needing re-examination
> -			sym->examined = 0;
> -			sym->endpos = token->pos;
> +			goto end;
>  		}
>  		return token;
>  	}
> @@ -763,10 +758,14 @@ static struct token *struct_union_enum_specifier(enum type type,
>  	}
>  
>  	sym = alloc_symbol(token->pos, type);
> +end:
>  	set_current_scope(sym);		// used by dissect
>  	token = parse(token->next, sym);
>  	ctx->ctype.base_type = sym;
>  	token =  expect(token, '}', "at end of specifier");
> +
> +	// Mark the structure as needing re-examination
> +	sym->examined = 0;
>  	sym->endpos = token->pos;

Hmm, this is not a simple 1:1 code movement.
Is the 'set_current_scope(sym);' or 'ctx->ctype.base_type = sym;'
relevant to this change? It is not mentioned in the commit
message (and I can't tell just looking at the patch text).

ATB,
Ramsay Jones

>  
>  	return token;
> diff --git a/validation/parsing/enum-attr.c b/validation/parsing/enum-attr.c
> index a962d8b417af..8d851a162135 100644
> --- a/validation/parsing/enum-attr.c
> +++ b/validation/parsing/enum-attr.c
> @@ -21,9 +21,9 @@ enum bad {
>  parsing/enum-attr.c:10:15: error: typename in expression
>  parsing/enum-attr.c:10:15: error: undefined identifier '__attribute__'
>  parsing/enum-attr.c:10:15: error: bad constant expression type
> -parsing/enum-attr.c:10:22: error: Expected } at end of struct-union-enum-specifier
> +parsing/enum-attr.c:10:22: error: Expected } at end of specifier
>  parsing/enum-attr.c:10:22: error: got 33
> -parsing/enum-attr.c:14:18: error: Expected } at end of struct-union-enum-specifier
> +parsing/enum-attr.c:14:18: error: Expected } at end of specifier
>  parsing/enum-attr.c:14:18: error: got __attribute__
>   * check-error-end
>   */
> 

Re: [PATCH v2 10/16] struct-attr: prepare to handle attributes at the end of struct definitions (1)

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 04:54:30PM +0000, Ramsay Jones wrote:
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > Type attributes for struct can be placed either just after the
> > keyword 'struct' or after the '}' ending its definition but this
> > later case is currently ignored.
> > 
> > Prepare the handling of this by factoring the code common to both
> > cases in a single place.
> > 
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > ---
> >  parse.c                        | 11 +++++------
> >  validation/parsing/enum-attr.c |  4 ++--
> >  2 files changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/parse.c b/parse.c
> > index d6343f0e48bf..99d810910dab 100644
> > --- a/parse.c
> > +++ b/parse.c
> > @@ -745,12 +745,7 @@ static struct token *struct_union_enum_specifier(enum type type,
> >  			if (sym->symbol_list)
> >  				error_die(token->pos, "redefinition of %s", show_typename (sym));
> >  			sym->pos = *repos;
> > -			token = parse(token->next, sym);
> > -			token = expect(token, '}', "at end of struct-union-enum-specifier");
> > -
> > -			// Mark the structure as needing re-examination
> > -			sym->examined = 0;
> > -			sym->endpos = token->pos;
> > +			goto end;
> >  		}
> >  		return token;
> >  	}
> > @@ -763,10 +758,14 @@ static struct token *struct_union_enum_specifier(enum type type,
> >  	}
> >  
> >  	sym = alloc_symbol(token->pos, type);
> > +end:
> >  	set_current_scope(sym);		// used by dissect
> >  	token = parse(token->next, sym);
> >  	ctx->ctype.base_type = sym;
> >  	token =  expect(token, '}', "at end of specifier");
> > +
> > +	// Mark the structure as needing re-examination
> > +	sym->examined = 0;
> >  	sym->endpos = token->pos;
> 
> Hmm, this is not a simple 1:1 code movement.
> Is the 'set_current_scope(sym);' or 'ctx->ctype.base_type = sym;'
> relevant to this change? It is not mentioned in the commit
> message (and I can't tell just looking at the patch text).

Good points, thanks.
For the 'set_current_scope(sym);', I'm not 100% sure it's OK and
it's to me to reason about it because it's done only for dissect.

Thinking a bit more about it, it should be moved just above the 'end:'.

Thanks,
-- Luc

[PATCH v2 11/16] struct-attr: prepare to handle attributes at the end of struct definitions (2)

[Luc Van Oostenryck]

Type attributes for struct can be placed either just after the
keyword 'struct' or after the '}' ending its definition but this
later case is currently ignored.

Prepare the handling of this by restructuring the code handling
struct specifiers, namely inverting the condition so that the
function can return early to make next patch's job easier.
---
 parse.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/parse.c b/parse.c
index 99d810910dab..a90dcf7f7e4a 100644
--- a/parse.c
+++ b/parse.c
@@ -738,16 +738,16 @@ static struct token *struct_union_enum_specifier(enum type type,
 		ctx->ctype.base_type = sym;
 		repos = &token->pos;
 		token = token->next;
-		if (match_op(token, '{')) {
-			// The following test is actually wrong for empty
-			// structs, but (1) they are not C99, (2) gcc does
-			// the same thing, and (3) it's easier.
-			if (sym->symbol_list)
-				error_die(token->pos, "redefinition of %s", show_typename (sym));
-			sym->pos = *repos;
-			goto end;
-		}
-		return token;
+		if (!match_op(token, '{'))
+			return token;
+
+		// The following test is actually wrong for empty
+		// structs, but (1) they are not C99, (2) gcc does
+		// the same thing, and (3) it's easier.
+		if (sym->symbol_list)
+			error_die(token->pos, "redefinition of %s", show_typename (sym));
+		sym->pos = *repos;
+		goto end;
 	}
 
 	// private struct/union/enum type
-- 
2.29.2

[PATCH v2 12/16] struct-attr: prepare to handle attributes at the end of struct definitions (3)

[Luc Van Oostenryck]

Type attributes for struct can be placed either just after the
keyword 'struct' or after the '}' ending its definition but this
later case is currently ignored.

Prepare the handling of this by having the 3 following cases in sequence:
1) a tag is present
2) no tag present but is followed by an opening brace
3) neither of these, so it's an error.
---
 parse.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/parse.c b/parse.c
index a90dcf7f7e4a..a67d567145d0 100644
--- a/parse.c
+++ b/parse.c
@@ -747,18 +747,15 @@ static struct token *struct_union_enum_specifier(enum type type,
 		if (sym->symbol_list)
 			error_die(token->pos, "redefinition of %s", show_typename (sym));
 		sym->pos = *repos;
-		goto end;
-	}
-
-	// private struct/union/enum type
-	if (!match_op(token, '{')) {
+	} else if (match_op(token, '{')) {
+		// private struct/union/enum type
+		sym = alloc_symbol(token->pos, type);
+	} else {
 		sparse_error(token->pos, "expected declaration");
 		ctx->ctype.base_type = &bad_ctype;
 		return token;
 	}
 
-	sym = alloc_symbol(token->pos, type);
-end:
 	set_current_scope(sym);		// used by dissect
 	token = parse(token->next, sym);
 	ctx->ctype.base_type = sym;
-- 
2.29.2

[PATCH v2 13/16] struct-attr: fix type attribute like 'struct __attr { ... }'

[Luc Van Oostenryck]

In a declaration like:
	struct <some attribute> { ... }
the attribute belong to the type but is currently handled as belonging
to the whole declaration.

Fix this by handling such attributes in a local 'decl_state' and
applying them once the closing '}' is reached.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/parse.c b/parse.c
index a67d567145d0..5f33ac178a45 100644
--- a/parse.c
+++ b/parse.c
@@ -719,10 +719,11 @@ static struct token *struct_union_enum_specifier(enum type type,
 	struct token *token, struct decl_state *ctx,
 	struct token *(*parse)(struct token *, struct symbol *))
 {
+	struct decl_state attr = { };
 	struct symbol *sym;
 	struct position *repos;
 
-	token = handle_attributes(token, ctx);
+	token = handle_attributes(token, &attr);
 	if (token_type(token) == TOKEN_IDENT) {
 		sym = lookup_symbol(token->ident, NS_STRUCT);
 		if (!sym ||
@@ -760,6 +761,7 @@ static struct token *struct_union_enum_specifier(enum type type,
 	token = parse(token->next, sym);
 	ctx->ctype.base_type = sym;
 	token =  expect(token, '}', "at end of specifier");
+	apply_ctype(token->pos, &sym->ctype, &attr.ctype);
 
 	// Mark the structure as needing re-examination
 	sym->examined = 0;
-- 
2.29.2

[PATCH v2 14/16] struct-attr: fix: do not ignore struct/union/enum type attributes

[Luc Van Oostenryck]

GCC's syntax for type attributes is specified as:
    An attribute specifier list may appear as part of a struct,
    union or enum specifier. It may go either immediately after
    the struct, union or enum keyword, or after the closing brace.
    The former syntax is preferred. Where attribute specifiers
    follow the closing brace, they are considered to relate to
    the structure, union or enumerated type defined, not to any
    enclosing declaration the type specifier appears in, and the type
    defined is not complete until after the attribute specifiers.
In the section about type attributes, it's also said:
    You may specify type attributes in an enum, struct or union type
    declaration or definition by placing them immediately after the
    struct, union or enum keyword. A less preferred syntax is to
    place them just past the closing curly brace of the definition.

So, while placing the attribute after the closing curly is not
preferred, it is cleary legal (and it seems to be much more popular
than placing them just after the struct, union or enum keyword).

However, currently sparse doesn't handle this correctly:
- these attributes are parsed in declaration_specifiers() and
  added to the current decl_state
- when the ';' ending the type declaration is reached, the plain
  struct/union/enum is used and the content of the decl_state is
  simply ignored.
- if the declaration is for a variable, then those attributes
  are assigned to the variable (but not to the type).

Fix this by calling handle_attribute() once we have reached the
closing '}' of a struct/union/enum definition and applying these
attributes, if any, directly to the current base type.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 parse.c                           | 2 ++
 validation/packed-bitfield3.c     | 1 -
 validation/packed-bitfield4.c     | 1 -
 validation/type-attribute-align.c | 1 -
 validation/type-attribute-as.c    | 1 -
 validation/type-attribute-mod.c   | 1 -
 6 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/parse.c b/parse.c
index 5f33ac178a45..0fd2bb473a47 100644
--- a/parse.c
+++ b/parse.c
@@ -761,6 +761,8 @@ static struct token *struct_union_enum_specifier(enum type type,
 	token = parse(token->next, sym);
 	ctx->ctype.base_type = sym;
 	token =  expect(token, '}', "at end of specifier");
+	attr.ctype.base_type = sym;
+	token = handle_attributes(token, &attr);
 	apply_ctype(token->pos, &sym->ctype, &attr.ctype);
 
 	// Mark the structure as needing re-examination
diff --git a/validation/packed-bitfield3.c b/validation/packed-bitfield3.c
index 6acff875299f..a8f740700967 100644
--- a/validation/packed-bitfield3.c
+++ b/validation/packed-bitfield3.c
@@ -25,5 +25,4 @@ _Static_assert(   sizeof(struct c) == sizeof(u64));
 
 /*
  * check-name: packed-bitfield3
- * check-known-to-fail
  */
diff --git a/validation/packed-bitfield4.c b/validation/packed-bitfield4.c
index 0342b2414b6e..1a956344f2d3 100644
--- a/validation/packed-bitfield4.c
+++ b/validation/packed-bitfield4.c
@@ -15,5 +15,4 @@ static int ld(struct s *s)
 /*
  * check-name: packed-bitfield4
  * check-description: Is check_access() OK with short packed bitfields?
- * check-known-to-fail
  */
diff --git a/validation/type-attribute-align.c b/validation/type-attribute-align.c
index d9358bff8327..5f3db14aceb1 100644
--- a/validation/type-attribute-align.c
+++ b/validation/type-attribute-align.c
@@ -16,5 +16,4 @@ void main(void)
 
 /*
  * check-name: type-attribute-align
- * check-known-to-fail
  */
diff --git a/validation/type-attribute-as.c b/validation/type-attribute-as.c
index b40b4e7dddf5..38f06b374e25 100644
--- a/validation/type-attribute-as.c
+++ b/validation/type-attribute-as.c
@@ -21,7 +21,6 @@ void main(void)
 
 /*
  * check-name: type-attribute-as
- * check-known-to-fail
  *
  * check-error-start
 type-attribute-as.c:16:15: warning: incorrect type in argument 1 (different address spaces)
diff --git a/validation/type-attribute-mod.c b/validation/type-attribute-mod.c
index 0e7b166a4aec..d55011dfa09a 100644
--- a/validation/type-attribute-mod.c
+++ b/validation/type-attribute-mod.c
@@ -14,7 +14,6 @@ void main(void)
 
 /*
  * check-name: type-attribute-mod
- * check-known-to-fail
  *
  * check-error-start
 type-attribute-mod.c:12:9: warning: dereference of noderef expression
-- 
2.29.2

[PATCH v2 15/16] packed: no out-of-bound access of packed bitfields

[Luc Van Oostenryck]

There is (at least) 2 ways by which packed bitfields doesn't
follow normal layout/access rules and as consequence can't (always)
be accessed the usual way (load the whole underlying word, then shift
and mask to isolate the bitfield).

At least two different cases are a concern:
1) there is no padding at the end of a bitfield sequence. For example,
   the following struct is only 3 bytes width:
	struct s {
		int f:24;
	} __packed;
   So, trying to access the bitfield by first doing a 32-bit load
   will create an out-of-bound access.

2) a bitfield smaller than one word may need more than one word to be
   accessed. For example, with the following struct
	struct {
		int a:5;
		int f:30;
		int z:5;
	} __packed;
   the bitfield 'f', while smaller than one 32-bit word, can't be accessed
   with a single 32-bit access.

At machine level, these bitfields should be accessed with several, possibly
smaller, loads and their corresponding values reconstructed form these,
making things much more complicated than for non-packed bitfields.

But at IR level, things can be a little more flexible and things can stay
simple by using sub-word or super-word accesses (until these need to
be lowered to be usable at machine level). In other words, the example here
can be safely accessed with respectively a 24-bit and a 40-bit load.
This is what is done in this patch.
---
 linearize.c | 13 +++++++++++--
 symbol.h    |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/linearize.c b/linearize.c
index 0250c6bb17ef..e80715ab2458 100644
--- a/linearize.c
+++ b/linearize.c
@@ -977,8 +977,17 @@ static struct symbol *bitfield_base_type(struct symbol *sym)
 	if (sym) {
 		if (sym->type == SYM_NODE)
 			base = base->ctype.base_type;
-		if (base->type == SYM_BITFIELD)
-			return base->ctype.base_type;
+		if (base->type == SYM_BITFIELD) {
+			base = base->ctype.base_type;
+			if (sym->packed) {
+				int size = bits_to_bytes(sym->bit_offset + sym->bit_size);
+				sym = __alloc_symbol(0);
+				*sym = *base;
+				sym->bit_size = bytes_to_bits(size);
+				return sym;
+			}
+			return base;
+		}
 	}
 	return sym;
 }
diff --git a/symbol.h b/symbol.h
index 5c5a7f12affa..866d57522f49 100644
--- a/symbol.h
+++ b/symbol.h
@@ -192,6 +192,7 @@ struct symbol {
 					accessed:1,
 					builtin:1,
 					torename:1,
+					packed:1,
 					transparent_union:1;
 			int		rank:3;	// arithmetic's rank
 			struct expression *array_size;
-- 
2.29.2

Re: [PATCH v2 15/16] packed: no out-of-bound access of packed bitfields

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> There is (at least) 2 ways by which packed bitfields doesn't
> follow normal layout/access rules and as consequence can't (always)
> be accessed the usual way (load the whole underlying word, then shift
> and mask to isolate the bitfield).
> 
> At least two different cases are a concern:
> 1) there is no padding at the end of a bitfield sequence. For example,
>    the following struct is only 3 bytes width:
> 	struct s {
> 		int f:24;
> 	} __packed;
>    So, trying to access the bitfield by first doing a 32-bit load
>    will create an out-of-bound access.
> 
> 2) a bitfield smaller than one word may need more than one word to be
>    accessed. For example, with the following struct
> 	struct {
> 		int a:5;
> 		int f:30;
> 		int z:5;
> 	} __packed;
>    the bitfield 'f', while smaller than one 32-bit word, can't be accessed
>    with a single 32-bit access.
> 
> At machine level, these bitfields should be accessed with several, possibly
> smaller, loads and their corresponding values reconstructed form these,

s/form/from/

> making things much more complicated than for non-packed bitfields.
> 
> But at IR level, things can be a little more flexible and things can stay
> simple by using sub-word or super-word accesses (until these need to
> be lowered to be usable at machine level). In other words, the example here
> can be safely accessed with respectively a 24-bit and a 40-bit load.
> This is what is done in this patch.

Hmm, I didn't know the IR could represent this! ;-)
Is the 'lowering' code already present? Maybe next patch.

ATB,
Ramsay Jones

> ---
>  linearize.c | 13 +++++++++++--
>  symbol.h    |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/linearize.c b/linearize.c
> index 0250c6bb17ef..e80715ab2458 100644
> --- a/linearize.c
> +++ b/linearize.c
> @@ -977,8 +977,17 @@ static struct symbol *bitfield_base_type(struct symbol *sym)
>  	if (sym) {
>  		if (sym->type == SYM_NODE)
>  			base = base->ctype.base_type;
> -		if (base->type == SYM_BITFIELD)
> -			return base->ctype.base_type;
> +		if (base->type == SYM_BITFIELD) {
> +			base = base->ctype.base_type;
> +			if (sym->packed) {
> +				int size = bits_to_bytes(sym->bit_offset + sym->bit_size);
> +				sym = __alloc_symbol(0);
> +				*sym = *base;
> +				sym->bit_size = bytes_to_bits(size);
> +				return sym;
> +			}
> +			return base;
> +		}
>  	}
>  	return sym;
>  }
> diff --git a/symbol.h b/symbol.h
> index 5c5a7f12affa..866d57522f49 100644
> --- a/symbol.h
> +++ b/symbol.h
> @@ -192,6 +192,7 @@ struct symbol {
>  					accessed:1,
>  					builtin:1,
>  					torename:1,
> +					packed:1,
>  					transparent_union:1;
>  			int		rank:3;	// arithmetic's rank
>  			struct expression *array_size;
> 

Re: [PATCH v2 15/16] packed: no out-of-bound access of packed bitfields

[Luc Van Oostenryck]

> 
> Hmm, I didn't know the IR could represent this! ;-)

It was already used but I would prefer to avoid it.
For example, when copying a structure:
	struct s {
		char name[5];
	} s, d;

	...
	d = s;
will linearize into a single 40-bit load + store.
In this case, it's quite OK because it directly translate to
a memcpy().

> Is the 'lowering' code already present? Maybe next patch.

I had several versions, all more ugly than the others. It's why
I ended with this 'OK, keep things simple for now'.
Also, there is several ways of doing this and I'm not convinced
of which one  should be used. Worse, the case:
	struct {
		a:10;
		f:14;
	};
should probably not be handled like the case:
	struct {
		a:5;
		f:30;
		z:5;
	};
since the problems are different (the first one is just a question
of not doing an out-of-bound access, while for the second case we
have a field not wider than 4-bytes but which can't be accessed 
in less than 5 bytes).
 
-- Luc

[PATCH v2 16/16] packed: add support for __packed struct

[Luc Van Oostenryck]

Currently, the attribute 'packed' is ignored
---
 parse.c                       |  5 ++++-
 symbol.c                      | 12 +++++++++---
 symbol.h                      |  1 +
 validation/packed-bitfield0.c |  1 -
 validation/packed-bitfield1.c |  1 -
 validation/packed-bitfield2.c |  1 -
 validation/packed-bitfield5.c |  1 -
 validation/packed-deref0.c    |  1 -
 validation/packed-struct.c    |  1 -
 9 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/parse.c b/parse.c
index 0fd2bb473a47..42e48620bd5e 100644
--- a/parse.c
+++ b/parse.c
@@ -764,6 +764,7 @@ static struct token *struct_union_enum_specifier(enum type type,
 	attr.ctype.base_type = sym;
 	token = handle_attributes(token, &attr);
 	apply_ctype(token->pos, &sym->ctype, &attr.ctype);
+	sym->packed = attr.packed;
 
 	// Mark the structure as needing re-examination
 	sym->examined = 0;
@@ -1088,8 +1089,10 @@ static struct token *ignore_attribute(struct token *token, struct symbol *attr,
 
 static struct token *attribute_packed(struct token *token, struct symbol *attr, struct decl_state *ctx)
 {
-	if (!ctx->ctype.alignment)
+	if (!ctx->ctype.alignment) {
 		ctx->ctype.alignment = 1;
+		ctx->packed = 1;
+	}
 	return token;
 }
 
diff --git a/symbol.c b/symbol.c
index 1a083fb8432c..aa02c8c5ad80 100644
--- a/symbol.c
+++ b/symbol.c
@@ -88,6 +88,7 @@ struct struct_union_info {
 	unsigned long bit_size;
 	int align_size;
 	char has_flex_array;
+	bool packed;
 	struct symbol *flex_array;
 };
 
@@ -120,6 +121,7 @@ static int bitfield_base_size(struct symbol *sym)
 static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 {
 	unsigned long bit_size, align_bit_mask;
+	unsigned long alignment;
 	int base_size;
 
 	bit_size = info->bit_size;
@@ -136,7 +138,8 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 		info->flex_array = sym;
 	}
 
-	align_bit_mask = bytes_to_bits(sym->ctype.alignment) - 1;
+	alignment = info->packed ? 1 : sym->ctype.alignment;
+	align_bit_mask = bytes_to_bits(alignment) - 1;
 
 	/*
 	 * Bitfields have some very special rules..
@@ -147,7 +150,7 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 		// Zero-width fields just fill up the unit.
 		int width = base_size ? : (bit_offset ? room : 0);
 
-		if (width > room) {
+		if (width > room && !info->packed) {
 			bit_size = (bit_size + align_bit_mask) & ~align_bit_mask;
 			bit_offset = 0;
 		}
@@ -157,6 +160,8 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 		info->bit_size = bit_size + width;
 		// warning (sym->pos, "bitfield: offset=%d:%d  size=:%d", sym->offset, sym->bit_offset, width);
 
+		if (info->packed && sym->type == SYM_NODE)
+			sym->packed = 1;
 		return;
 	}
 
@@ -173,6 +178,7 @@ static void lay_out_struct(struct symbol *sym, struct struct_union_info *info)
 static struct symbol * examine_struct_union_type(struct symbol *sym, int advance)
 {
 	struct struct_union_info info = {
+		.packed = sym->packed,
 		.max_align = 1,
 		.bit_size = 0,
 		.align_size = 1
@@ -191,7 +197,7 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance
 			sparse_error(info.flex_array->pos, "flexible array member '%s' is not last", show_ident(info.flex_array->ident));
 		examine_symbol_type(member);
 
-		if (member->ctype.alignment > info.max_align) {
+		if (member->ctype.alignment > info.max_align && !sym->packed) {
 			// Unnamed bitfields do not affect alignment.
 			if (member->ident || !is_bitfield_type(member))
 				info.max_align = member->ctype.alignment;
diff --git a/symbol.h b/symbol.h
index 866d57522f49..15b21452c934 100644
--- a/symbol.h
+++ b/symbol.h
@@ -112,6 +112,7 @@ struct decl_state {
 	unsigned char prefer_abstract;
 	unsigned char autotype;
 	unsigned char forced;
+	unsigned char packed;
 };
 
 struct pseudo;
diff --git a/validation/packed-bitfield0.c b/validation/packed-bitfield0.c
index 907500dedbf0..6d92fb2862eb 100644
--- a/validation/packed-bitfield0.c
+++ b/validation/packed-bitfield0.c
@@ -63,5 +63,4 @@ int main(void)
 
 /*
  * check-name: packed-bitfield0
- * check-known-to-fail
  */
diff --git a/validation/packed-bitfield1.c b/validation/packed-bitfield1.c
index 208a3dc5127c..b7b575ce6922 100644
--- a/validation/packed-bitfield1.c
+++ b/validation/packed-bitfield1.c
@@ -24,5 +24,4 @@ static int foo(struct s *ptr)
 
 /*
  * check-name: packed-bitfield1
- * check-known-to-fail
  */
diff --git a/validation/packed-bitfield2.c b/validation/packed-bitfield2.c
index 4587ebec5c90..244204c2dd35 100644
--- a/validation/packed-bitfield2.c
+++ b/validation/packed-bitfield2.c
@@ -12,5 +12,4 @@ _Static_assert(sizeof(struct bf2) == 8);
 
 /*
  * check-name: packed-bitfield2
- * check-known-to-fail
  */
diff --git a/validation/packed-bitfield5.c b/validation/packed-bitfield5.c
index 8f44d4c2c277..87dbf9c221a1 100644
--- a/validation/packed-bitfield5.c
+++ b/validation/packed-bitfield5.c
@@ -17,5 +17,4 @@ static int ld(struct s *s)
 /*
  * check-name: packed-bitfield5
  * check-description: is check_access() OK with 'overlapping' packed bitfields?
- * check-known-to-fail
  */
diff --git a/validation/packed-deref0.c b/validation/packed-deref0.c
index 865ad68a4f37..d48ad1ac7505 100644
--- a/validation/packed-deref0.c
+++ b/validation/packed-deref0.c
@@ -20,5 +20,4 @@ static void bar(obj_t o)
 
 /*
  * check-name: packed-deref0
- * check-known-to-fail
  */
diff --git a/validation/packed-struct.c b/validation/packed-struct.c
index 5039be4d0b45..253750063ffd 100644
--- a/validation/packed-struct.c
+++ b/validation/packed-struct.c
@@ -29,5 +29,4 @@ _Static_assert(   sizeof(struct c) == 6, "size struct");
 
 /*
  * check-name: packed-struct
- * check-known-to-fail
  */
-- 
2.29.2

Re: [PATCH 00/16] support __packed struct

[Ramsay Jones]

On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> During parsing, Sparse recognizes the attribute 'packed' but this
> attribute is otherwise ignored for several reasons:
> 1) the attribute in 'struct __attr { ... }' is wrongly handled as
>    belonging to the whole declaration but it should belong to the type,
> 2) the attribute in 'struct <name> { ... } __attr;' is simply ignored,
> 3) the layout of packed bitfields need special care.
> 
> This series contains 2 parts:
> 1) handling of type attributes
> 2) correct layout of packed structs, including packed bitfields.
> 
> 
> This series is also available for review and testing at:
>   git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git packed-v2

I left a couple of minor comments, but (apart from patch #16) this
otherwise LGTM.

Patch #16 also looks good, but I would need to study it a bit more
than I have time available to be totally happy. It does not seem
to be handling the 'lowering' of 'odd bit-sized' symbols created in
the previous patch (to answer my own question), so I would have to
apply the patches (or fetch the above branch) to study some more.

Hope you had a good holiday.

ATB,
Ramsay Jones

Re: [PATCH 00/16] support __packed struct

[Luc Van Oostenryck]

On Mon, Dec 28, 2020 at 05:18:50PM +0000, Ramsay Jones wrote:
> 
> 
> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
> > During parsing, Sparse recognizes the attribute 'packed' but this
> > attribute is otherwise ignored for several reasons:
> > 1) the attribute in 'struct __attr { ... }' is wrongly handled as
> >    belonging to the whole declaration but it should belong to the type,
> > 2) the attribute in 'struct <name> { ... } __attr;' is simply ignored,
> > 3) the layout of packed bitfields need special care.
> > 
> > This series contains 2 parts:
> > 1) handling of type attributes
> > 2) correct layout of packed structs, including packed bitfields.
> > 
> > 
> > This series is also available for review and testing at:
> >   git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git packed-v2
> 
> I left a couple of minor comments, but (apart from patch #16) this
> otherwise LGTM.
> 
> Patch #16 also looks good, but I would need to study it a bit more
> than I have time available to be totally happy. It does not seem
> to be handling the 'lowering' of 'odd bit-sized' symbols created in
> the previous patch (to answer my own question), so I would have to
> apply the patches (or fetch the above branch) to study some more.

Well, patch #16 doesn't contain the lowering, it kinda just enables
the last bits to support (without out-of-bound access) packed structures
including packed bitfields.

> Hope you had a good holiday.

Thank you. In truth, it was strange and quiet, very quiet.

I wish you all the best for 2021!

-- Luc

Re: [PATCH 00/16] support __packed struct

[Jacob Keller]

On 12/28/2020 1:33 PM, Luc Van Oostenryck wrote:
> On Mon, Dec 28, 2020 at 05:18:50PM +0000, Ramsay Jones wrote:
>>
>>
>> On 26/12/2020 17:51, Luc Van Oostenryck wrote:
>>> During parsing, Sparse recognizes the attribute 'packed' but this
>>> attribute is otherwise ignored for several reasons:
>>> 1) the attribute in 'struct __attr { ... }' is wrongly handled as
>>>    belonging to the whole declaration but it should belong to the type,
>>> 2) the attribute in 'struct <name> { ... } __attr;' is simply ignored,
>>> 3) the layout of packed bitfields need special care.
>>>
>>> This series contains 2 parts:
>>> 1) handling of type attributes
>>> 2) correct layout of packed structs, including packed bitfields.
>>>
>>>
>>> This series is also available for review and testing at:
>>>   git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git packed-v2
>>
>> I left a couple of minor comments, but (apart from patch #16) this
>> otherwise LGTM.
>>
>> Patch #16 also looks good, but I would need to study it a bit more
>> than I have time available to be totally happy. It does not seem
>> to be handling the 'lowering' of 'odd bit-sized' symbols created in
>> the previous patch (to answer my own question), so I would have to
>> apply the patches (or fetch the above branch) to study some more.
> 
> Well, patch #16 doesn't contain the lowering, it kinda just enables
> the last bits to support (without out-of-bound access) packed structures
> including packed bitfields.
> 


What does the 'lowering' gain us? Or, in other words, what is still
missing after this series?

>> Hope you had a good holiday.
> 
> Thank you. In truth, it was strange and quiet, very quiet.
> 
> I wish you all the best for 2021!
> 
> -- Luc
> 

Re: [PATCH 00/16] support __packed struct

[Luc Van Oostenryck]

On Tue, Jan 05, 2021 at 09:56:05AM -0800, Jacob Keller wrote:
> On 12/28/2020 1:33 PM, Luc Van Oostenryck wrote:
> > On Mon, Dec 28, 2020 at 05:18:50PM +0000, Ramsay Jones wrote:
> >>
> >> Patch #16 also looks good, but I would need to study it a bit more
> >> than I have time available to be totally happy. It does not seem
> >> to be handling the 'lowering' of 'odd bit-sized' symbols created in
> >> the previous patch (to answer my own question), so I would have to
> >> apply the patches (or fetch the above branch) to study some more.
> > 
> > Well, patch #16 doesn't contain the lowering, it kinda just enables
> > the last bits to support (without out-of-bound access) packed structures
> > including packed bitfields.
> > 
> 
> What does the 'lowering' gain us? Or, in other words, what is still
> missing after this series?

As a static checker, nothing is missing and the series is now mainlined.

The 'lowering' only matters if you want to somehow translate the
instructions used in the IR (Intermediate Representation) into
instructions for a more concrete machine, because now there are things
like: 'do a load of a 5-byte word'.

-- Luc

Re: [PATCH 00/16] support __packed struct

[Jacob Keller]

On 1/5/2021 12:39 PM, Luc Van Oostenryck wrote:
> On Tue, Jan 05, 2021 at 09:56:05AM -0800, Jacob Keller wrote:
>> On 12/28/2020 1:33 PM, Luc Van Oostenryck wrote:
>>> On Mon, Dec 28, 2020 at 05:18:50PM +0000, Ramsay Jones wrote:
>>>>
>>>> Patch #16 also looks good, but I would need to study it a bit more
>>>> than I have time available to be totally happy. It does not seem
>>>> to be handling the 'lowering' of 'odd bit-sized' symbols created in
>>>> the previous patch (to answer my own question), so I would have to
>>>> apply the patches (or fetch the above branch) to study some more.
>>>
>>> Well, patch #16 doesn't contain the lowering, it kinda just enables
>>> the last bits to support (without out-of-bound access) packed structures
>>> including packed bitfields.
>>>
>>
>> What does the 'lowering' gain us? Or, in other words, what is still
>> missing after this series?
> 
> As a static checker, nothing is missing and the series is now mainlined.
> 
> The 'lowering' only matters if you want to somehow translate the
> instructions used in the IR (Intermediate Representation) into
> instructions for a more concrete machine, because now there are things
> like: 'do a load of a 5-byte word'.
> 
> -- Luc
> 

Ok thanks, that makes sense.

- Jake

Re: [PATCH 00/16] support __packed struct

[Jacob Keller]

On 12/26/2020 9:51 AM, Luc Van Oostenryck wrote:
> During parsing, Sparse recognizes the attribute 'packed' but this
> attribute is otherwise ignored for several reasons:
> 1) the attribute in 'struct __attr { ... }' is wrongly handled as
>    belonging to the whole declaration but it should belong to the type,
> 2) the attribute in 'struct <name> { ... } __attr;' is simply ignored,
> 3) the layout of packed bitfields need special care.
> 
> This series contains 2 parts:
> 1) handling of type attributes
> 2) correct layout of packed structs, including packed bitfields.
> 
> 
> This series is also available for review and testing at:
>   git://git.kernel.org/pub/scm/devel/sparse/sparse-dev.git packed-v2
> 

This series looks good to me, and it resolves the issues I saw a few
weeks ago with size.

Thanks,
Jake

> 
> Changes since v1:
> * fix layout of packed bitfields
> 
> Luc Van Oostenryck (16):
>   add testcases for dubious enum values
>   add testcases for exotic enum values
>   add testcases for enum attributes
>   add testcases for type attributes
>   add testcases for packed structures
>   add testcases for packed bitfields
>   apply_ctype: use self-explanatory argument name
>   apply_ctype: reverse the order of arguments
>   apply_ctype: move up its declaration
>   struct-attr: prepare to handle attributes at the end of struct definitions (1)
>   struct-attr: prepare to handle attributes at the end of struct definitions (2)
>   struct-attr: prepare to handle attributes at the end of struct definitions (3)
>   struct-attr: fix type attribute like 'struct __attr { ... }'
>   struct-attr: fix: do not ignore struct/union/enum type attributes
>   packed: no out-of-bound access of packed bitfields
>   packed: add support for __packed struct
> 
>  linearize.c                       | 13 +++++-
>  parse.c                           | 71 ++++++++++++++++---------------
>  symbol.c                          | 12 ++++--
>  symbol.h                          |  2 +
>  validation/enum-type-dubious.c    | 18 ++++++++
>  validation/enum-type-exotic.c     | 28 ++++++++++++
>  validation/packed-bitfield0.c     | 66 ++++++++++++++++++++++++++++
>  validation/packed-bitfield1.c     | 27 ++++++++++++
>  validation/packed-bitfield2.c     | 15 +++++++
>  validation/packed-bitfield3.c     | 28 ++++++++++++
>  validation/packed-bitfield4.c     | 18 ++++++++
>  validation/packed-bitfield5.c     | 20 +++++++++
>  validation/packed-deref0.c        | 23 ++++++++++
>  validation/packed-struct.c        | 32 ++++++++++++++
>  validation/parsing/enum-attr.c    | 29 +++++++++++++
>  validation/type-attribute-align.c | 19 +++++++++
>  validation/type-attribute-as.c    | 33 ++++++++++++++
>  validation/type-attribute-mod.c   | 21 +++++++++
>  validation/type-attribute-qual.c  | 12 ++++++
>  19 files changed, 448 insertions(+), 39 deletions(-)
>  create mode 100644 validation/enum-type-dubious.c
>  create mode 100644 validation/enum-type-exotic.c
>  create mode 100644 validation/packed-bitfield0.c
>  create mode 100644 validation/packed-bitfield1.c
>  create mode 100644 validation/packed-bitfield2.c
>  create mode 100644 validation/packed-bitfield3.c
>  create mode 100644 validation/packed-bitfield4.c
>  create mode 100644 validation/packed-bitfield5.c
>  create mode 100644 validation/packed-deref0.c
>  create mode 100644 validation/packed-struct.c
>  create mode 100644 validation/parsing/enum-attr.c
>  create mode 100644 validation/type-attribute-align.c
>  create mode 100644 validation/type-attribute-as.c
>  create mode 100644 validation/type-attribute-mod.c
>  create mode 100644 validation/type-attribute-qual.c
> 
> 
> base-commit: 1b896707d95982c7c9cdd5cd0ab4afd80f766a94
> Cc: Jacob Keller <jacob.e.keller@intel.com>
>