Change in coreboot[master]: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42196 )

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

assert.h: Do not use __FILE__ nor __LINE__ on timeless builds

When refactoring, one can move code around quite a bit while preserving
reproducibility, unless there is an assert-style macro somewhere... As
these macros use __FILE__ and __LINE__, just moving them is enough to
change the resulting binary, making timeless builds rather useless.

To improve reproducibility, do not use __FILE__ nor __LINE__ inside the
assert-style macros. Instead, use hardcoded values. Plus, mention that
timeless builds lack such information in place of the file name, so that
grepping for the printed string directs one towards this commit. And for
the immutable line number, we can use 404: line number not found :-)

Change-Id: Id42d7121b6864759c042f8e4e438ee77a8ac0b41
Signed-off-by: Angel Pons 
---
M src/include/assert.h
1 file changed, 33 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/42196/1

diff --git a/src/include/assert.h b/src/include/assert.h
index f656d81..da8906d 100644
--- a/src/include/assert.h
+++ b/src/include/assert.h
@@ -4,6 +4,7 @@
 #define __ASSERT_H__

#include <arch/hlt.h>
+#include <build.h>
 #include <console/console.h>

/* TODO: Fix vendorcode headers to not define macros coreboot uses or to be more
@@ -12,31 +13,43 @@
 #undef ASSERT
 #endif

+/* Do not use filenames nor line numbers on timeless builds, to preserve reproducibility */
+#if COREBOOT_VERSION_TIMESTAMP == 0
+#define __ASSERT_FILE__ "(filenames not available on timeless builds)"
+#define __ASSERT_LINE__ 404
+#else
+#define __ASSERT_FILE__ __FILE__
+#define __ASSERT_LINE__ __LINE__
+#endif
+
 /* GCC and CAR versions */
-#define ASSERT(x) {						\
-	if (!(x)) {						\
-		printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'"	\
-			", line %d\n", __FILE__, __LINE__);	\
-		if (CONFIG(FATAL_ASSERTS))		\
-			hlt();					\
-	}							\
+#define ASSERT(x) {							\
+	if (!(x)) {							\
+		printk(BIOS_EMERG,					\
+			"ASSERTION ERROR: file '%s', line %d\n",	\
+			__ASSERT_FILE__, __ASSERT_LINE__);		\
+		if (CONFIG(FATAL_ASSERTS))				\
+			hlt();						\
+	}								\
 }

-#define ASSERT_MSG(x, msg) {					\
-	if (!(x)) {						\
-		printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'"	\
-			", line %d\n", __FILE__, __LINE__);	\
-		printk(BIOS_EMERG, "%s", msg);                  \
-		if (CONFIG(FATAL_ASSERTS))			\
-			hlt();					\
-	}							\
+#define ASSERT_MSG(x, msg) {						\
+	if (!(x)) {							\
+		printk(BIOS_EMERG,					\
+			"ASSERTION ERROR: file '%s', line %d\n",	\
+			__ASSERT_FILE__, __ASSERT_LINE__);		\
+		printk(BIOS_EMERG, "%s", msg);				\
+		if (CONFIG(FATAL_ASSERTS))				\
+			hlt();						\
+	}								\
 }

-#define BUG() {							\
-	printk(BIOS_EMERG, "ERROR: BUG ENCOUNTERED at file '%s'"\
-		", line %d\n", __FILE__, __LINE__);		\
-	if (CONFIG(FATAL_ASSERTS))			\
-		hlt();						\
+#define BUG() {								\
+	printk(BIOS_EMERG,						\
+		"ERROR: BUG ENCOUNTERED at file '%s', line %d\n",	\
+		__ASSERT_FILE__, __ASSERT_LINE__);			\
+	if (CONFIG(FATAL_ASSERTS))					\
+		hlt();							\
 }

#define assert(statement)	ASSERT(statement)

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42196 )

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h@47 
PS1, Line 47: #define BUG() {								\
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()

[Nico Huber (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 1: Code-Review+1

[Paul Menzel (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 1: Code-Review+1

[Julius Werner (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h@7 
PS1, Line 7: #include <build.h>
Including <build.h> is always a dependency problem since it is auto-generated. Normally we manually add this dependency to .c files including it, but here you can't do this because this is in itself a header. We could make it a dependency for everything (like <config.h>) but that would be nice to avoid to help with build parallelization.

Since you only want to know *if* we're a timeless build here, not any actual build information from genbuild, I think it would be best to sidestep <build.h>... just put something like this in the Makefile:

ifeq ($(BUILD_TIMELESS),1)
 CPPFLAGS_common += -D__TIMELESS__
 endif

and then to make it pretty in <rules.h>:

#if defined(__TIMELESS__)
 #define ENV_TIMELESS 1
 #else
 #define ENV_TIMELESS 0
 #endif

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h@22 
PS1, Line 22: #define __ASSERT_LINE__ __LINE__
Just to be sure, have you double-checked that this still outputs the correct file and line number in non-timeless builds? Macro replacement order can be tricky and I wouldn't know off-hand how it works out here... wouldn't want every assert to say "file 'src/include/assert.h', line 30".

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, Paul Menzel, Julius Werner,

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

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

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

assert.h: Do not use __FILE__ nor __LINE__ on timeless builds

When refactoring, one can move code around quite a bit while preserving
reproducibility, unless there is an assert-style macro somewhere... As
these macros use __FILE__ and __LINE__, just moving them is enough to
change the resulting binary, making timeless builds rather useless.

To improve reproducibility, do not use __FILE__ nor __LINE__ inside the
assert-style macros. Instead, use hardcoded values. Plus, mention that
timeless builds lack such information in place of the file name, so that
grepping for the printed string directs one towards this commit. And for
the immutable line number, we can use 404: line number not found :-)

Change-Id: Id42d7121b6864759c042f8e4e438ee77a8ac0b41
Signed-off-by: Angel Pons 
---
M Makefile.inc
M src/include/assert.h
M src/include/rules.h
3 files changed, 38 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/42196/2

[Angel Pons (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h@7 
PS1, Line 7: #include <build.h>
> Including <build.h> is always a dependency problem since it is auto-generated. […]

Done

https://review.coreboot.org/c/coreboot/+/42196/1/src/include/assert.h@22 
PS1, Line 22: #define __ASSERT_LINE__ __LINE__
> Just to be sure, have you double-checked that this still outputs the correct file and line number in […]

I found a better approach: the #line directive.

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42196 )

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h@42 
PS2, Line 42: #define BUG() {								\
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()

[Julius Werner (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h@17 
PS2, Line 17: #line 404 "(filenames not available on timeless builds)"
So does this actually work? From my understanding of what the #line directive does, it shouldn't... you're just telling the preprocessor that *this very line* is line 404, but it will still keep counting further after that. It doesn't just return 404 in all future __LINE__ macros.

Besides, I think you're changing the line number and file name for the assert.h file here. But the __LINE__ macro is evaluated in the context of the file where the ASSERT() was invoked, so I don't think you would affect that at all with this. I think if you want to do what you want to do, you have to go with your previous approach (I tested it now, I think that should work and still give you the original line number despite the double replacement).

[Angel Pons (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h@17 
PS2, Line 17: #line 404 "(filenames not available on timeless builds)"
> So does this actually work? From my understanding of what the #line directive does, it shouldn't... […]

Right, this won't work. I'll roll back to the previous approach

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, Paul Menzel, Julius Werner,

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

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

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

assert.h: Do not use __FILE__ nor __LINE__ on timeless builds

When refactoring, one can move code around quite a bit while preserving
reproducibility, unless there is an assert-style macro somewhere... As
these macros use __FILE__ and __LINE__, just moving them is enough to
change the resulting binary, making timeless builds rather useless.

To improve reproducibility, do not use __FILE__ nor __LINE__ inside the
assert-style macros. Instead, use hardcoded values. Plus, mention that
timeless builds lack such information in place of the file name, so that
grepping for the printed string directs one towards this commit. And for
the immutable line number, we can use 404: line number not found :-)

Change-Id: Id42d7121b6864759c042f8e4e438ee77a8ac0b41
Signed-off-by: Angel Pons 
---
M Makefile.inc
M src/include/assert.h
M src/include/rules.h
3 files changed, 42 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/42196/3

[Angel Pons (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/2/src/include/assert.h@17 
PS2, Line 17: #line 404 "(filenames not available on timeless builds)"
> Right, this won't work. […]

Done. I restored the original approach.

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42196 )

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42196/3/src/include/assert.h 
File src/include/assert.h:

https://review.coreboot.org/c/coreboot/+/42196/3/src/include/assert.h@44 
PS3, Line 44: #define BUG() {								\
Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()

[Julius Werner (Code Review)]

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

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42196 )

Change subject: assert.h: Do not use __FILE__ nor __LINE__ on timeless builds
......................................................................

assert.h: Do not use __FILE__ nor __LINE__ on timeless builds

When refactoring, one can move code around quite a bit while preserving
reproducibility, unless there is an assert-style macro somewhere... As
these macros use __FILE__ and __LINE__, just moving them is enough to
change the resulting binary, making timeless builds rather useless.

To improve reproducibility, do not use __FILE__ nor __LINE__ inside the
assert-style macros. Instead, use hardcoded values. Plus, mention that
timeless builds lack such information in place of the file name, so that
grepping for the printed string directs one towards this commit. And for
the immutable line number, we can use 404: line number not found :-)

Change-Id: Id42d7121b6864759c042f8e4e438ee77a8ac0b41
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/42196
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M Makefile.inc
M src/include/assert.h
M src/include/rules.h
3 files changed, 42 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/Makefile.inc b/Makefile.inc
index 89bb3e4..a51b73d 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -411,6 +411,10 @@
 CPPFLAGS_common += -I3rdparty
 CPPFLAGS_common += -D__BUILD_DIR__="$(obj)"

+ifeq ($(BUILD_TIMELESS),1)
+CPPFLAGS_common += -D__TIMELESS__
+endif
+
 ifeq ($(CONFIG_PCI_OPTION_ROM_RUN_YABEL)$(CONFIG_PCI_OPTION_ROM_RUN_REALMODE),y)
 CPPFLAGS_ramstage += -Isrc/device/oprom/include
 endif
diff --git a/src/include/assert.h b/src/include/assert.h
index f656d81..8c19c1c 100644
--- a/src/include/assert.h
+++ b/src/include/assert.h
@@ -12,31 +12,41 @@
 #undef ASSERT
 #endif

+/* Do not use filenames nor line numbers on timeless builds, to preserve reproducibility */
+#if ENV_TIMELESS
+#define __ASSERT_FILE__ "(filenames not available on timeless builds)"
+#define __ASSERT_LINE__ 404
+#else
+#define __ASSERT_FILE__ __FILE__
+#define __ASSERT_LINE__ __LINE__
+#endif
+
 /* GCC and CAR versions */
-#define ASSERT(x) {						\
-	if (!(x)) {						\
-		printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'"	\
-			", line %d\n", __FILE__, __LINE__);	\
-		if (CONFIG(FATAL_ASSERTS))		\
-			hlt();					\
-	}							\
+#define ASSERT(x) {							\
+	if (!(x)) {							\
+		printk(BIOS_EMERG,					\
+			"ASSERTION ERROR: file '%s', line %d\n",	\
+			__ASSERT_FILE__, __ASSERT_LINE__);		\
+		if (CONFIG(FATAL_ASSERTS))				\
+			hlt();						\
+	}								\
 }
-
-#define ASSERT_MSG(x, msg) {					\
-	if (!(x)) {						\
-		printk(BIOS_EMERG, "ASSERTION ERROR: file '%s'"	\
-			", line %d\n", __FILE__, __LINE__);	\
-		printk(BIOS_EMERG, "%s", msg);                  \
-		if (CONFIG(FATAL_ASSERTS))			\
-			hlt();					\
-	}							\
+#define ASSERT_MSG(x, msg) {						\
+	if (!(x)) {							\
+		printk(BIOS_EMERG,					\
+			"ASSERTION ERROR: file '%s', line %d\n",	\
+			__ASSERT_FILE__, __ASSERT_LINE__);		\
+		printk(BIOS_EMERG, "%s", msg);				\
+		if (CONFIG(FATAL_ASSERTS))				\
+			hlt();						\
+	}								\
 }
-
-#define BUG() {							\
-	printk(BIOS_EMERG, "ERROR: BUG ENCOUNTERED at file '%s'"\
-		", line %d\n", __FILE__, __LINE__);		\
-	if (CONFIG(FATAL_ASSERTS))			\
-		hlt();						\
+#define BUG() {								\
+	printk(BIOS_EMERG,						\
+		"ERROR: BUG ENCOUNTERED at file '%s', line %d\n",	\
+		__ASSERT_FILE__, __ASSERT_LINE__);			\
+	if (CONFIG(FATAL_ASSERTS))					\
+		hlt();							\
 }

#define assert(statement)	ASSERT(statement)
diff --git a/src/include/rules.h b/src/include/rules.h
index 160829e..2cc54e7 100644
--- a/src/include/rules.h
+++ b/src/include/rules.h
@@ -3,6 +3,12 @@
 #ifndef _RULES_H
 #define _RULES_H

+#if defined(__TIMELESS__)
+#define ENV_TIMELESS 1
+#else
+#define ENV_TIMELESS 0
+#endif
+
 /* Useful helpers to tell whether the code is executing in bootblock,
  * romstage, ramstage or SMM.
  */