Change in coreboot[master]: include/list.h: Add support for GCC9+

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

include/list.h: Add support for GCC9+

When getting the address of a structure's member that is not on offset 0,
GCC9+ assumes that the address never can be NULL. However the code relied
on the fact that it can be NULL by letting the pointer intentionally
overflow.

Manually calculate the address using uintptr_t. This allows to gracefully
terminate the list_for_each MACRO instead of crashing at the end of the
list.

Tested on qemu-system-arm:
coreboot no longer crashed in the devicetree parser and is able to boot
Linux 5.5.

Change-Id: I0d569b59a23d1269f8575fcbbe92a5a6816aa1f7
Signed-off-by: Patrick Rudolph 
---
M src/include/list.h
1 file changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/44573/1

diff --git a/src/include/list.h b/src/include/list.h
index 3944878..6f0b54d 100644
--- a/src/include/list.h
+++ b/src/include/list.h
@@ -16,10 +16,10 @@
 // Insert list_node node before list_node before in a doubly linked list.
 void list_insert_before(struct list_node *node, struct list_node *before);

-#define list_for_each(ptr, head, member)                                \
-	for ((ptr) = container_of((head).next, typeof(*(ptr)), member); \
-		&((ptr)->member);                                       \
-		(ptr) = container_of((ptr)->member.next,                \
+#define list_for_each(ptr, head, member)                                      \
+	for ((ptr) = container_of((head).next, typeof(*(ptr)), member);       \
+		(uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member); \
+		(ptr) = container_of((ptr)->member.next,                      \
    		typeof(*(ptr)), member))

#endif /* __LIST_H__ */

[Patrick Rudolph (Code Review)]

Hello Nico Huber, Martin Roth, Patrick Georgi, Angel Pons, Arthur Heymans, Iru Cai (vimacs), HAOUAS Elyes,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/44573

to look at the new patch set (#2).

Change subject: include/list.h: Add support for GCC9+
......................................................................

include/list.h: Add support for GCC9+

When getting the address of a structure's member that is not on
offset 0, GCC9+ assumes that the address never can be NULL. However
the code relied on the fact that it can be NULL by letting the pointer
intentionally overflow.

Manually calculate the address using uintptr_t. This allows to
gracefully terminate the list_for_each MACRO instead of crashing at the
end of the list.

Tested on qemu-system-arm:
coreboot no longer crashed in the devicetree parser and is able to boot
Linux 5.5.

Change-Id: I0d569b59a23d1269f8575fcbbe92a5a6816aa1f7
Signed-off-by: Patrick Rudolph 
---
M src/include/list.h
1 file changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/44573/2

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 2: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44573/2/src/include/list.h 
File src/include/list.h:

https://review.coreboot.org/c/coreboot/+/44573/2/src/include/list.h@23 
PS2, Line 23: 			typeof(*(ptr)), member))
Untested, but we can simply avoid the overflow by adding another
variable:

#define list_for_each(ptr, head, member)                                \
            for (struct list_node *it = (head).next,                        \
                    (ptr) = container_of(it, typeof(*(ptr)), member);       \
                 it != NULL;                                                        \
                 it = it->next,                                             \
                    (ptr) = container_of(it, typeof(*(ptr)), member))

OTOH, `it` would ideally be declared outside the macro which would require
us to adapt all invocations.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/44573/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44573/2//COMMIT_MSG@10 
PS2, Line 10: never can
nit: can never

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 2:

Nice find.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

Good fix, thanks!

https://review.coreboot.org/c/coreboot/+/44573/2/src/include/list.h 
File src/include/list.h:

https://review.coreboot.org/c/coreboot/+/44573/2/src/include/list.h@23 
PS2, Line 23: 			typeof(*(ptr)), member))
> Untested, but we can simply avoid the overflow by adding another […]

This breaks when you try to nest two list_for_each(), because then you'll have two variables named 'it' (unless you try to do really complicated __COUNTER__ stuff).

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#3) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

include/list.h: Add support for GCC9+

When getting the address of a structure's member that is not on
offset 0, GCC9+ assumes that the address can never be NULL. However
the code relied on the fact that it can be NULL by letting the pointer
intentionally overflow.

Manually calculate the address using uintptr_t. This allows to
gracefully terminate the list_for_each MACRO instead of crashing at the
end of the list.

Tested on qemu-system-arm:
coreboot no longer crashed in the devicetree parser and is able to boot
Linux 5.5.

Change-Id: I0d569b59a23d1269f8575fcbbe92a5a6816aa1f7
Signed-off-by: Patrick Rudolph 
---
M src/include/list.h
1 file changed, 4 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/44573/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44573/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44573/2//COMMIT_MSG@10 
PS2, Line 10: never can
> nit: can never

Done

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 3: Code-Review+2

Will need a rebase once CB:47102 goes in

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

Patch Set 3:

CB:47102 was merged

[Patrick Rudolph (Code Review)]

Patrick Rudolph has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44573 )

Change subject: include/list.h: Add support for GCC9+
......................................................................

include/list.h: Add support for GCC9+

When getting the address of a structure's member that is not on
offset 0, GCC9+ assumes that the address can never be NULL. However
the code relied on the fact that it can be NULL by letting the pointer
intentionally overflow.

Manually calculate the address using uintptr_t. This allows to
gracefully terminate the list_for_each MACRO instead of crashing at the
end of the list.

Tested on qemu-system-arm:
coreboot no longer crashed in the devicetree parser and is able to boot
Linux 5.5.

Change-Id: I0d569b59a23d1269f8575fcbbe92a5a6816aa1f7
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44573
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Angel Pons 
---
M src/include/list.h
1 file changed, 4 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/include/list.h b/src/include/list.h
index 3944878..6f0b54d 100644
--- a/src/include/list.h
+++ b/src/include/list.h
@@ -16,10 +16,10 @@
 // Insert list_node node before list_node before in a doubly linked list.
 void list_insert_before(struct list_node *node, struct list_node *before);

-#define list_for_each(ptr, head, member)                                \
-	for ((ptr) = container_of((head).next, typeof(*(ptr)), member); \
-		&((ptr)->member);                                       \
-		(ptr) = container_of((ptr)->member.next,                \
+#define list_for_each(ptr, head, member)                                      \
+	for ((ptr) = container_of((head).next, typeof(*(ptr)), member);       \
+		(uintptr_t)ptr + (uintptr_t)offsetof(typeof(*(ptr)), member); \
+		(ptr) = container_of((ptr)->member.next,                      \
    		typeof(*(ptr)), member))

#endif /* __LIST_H__ */