Change in ...coreboot[master]: string.h: allow to compile strdup in romstage

[Thomas Heijligen (Code Review)]

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30844

Change subject: string.h: allow to compile strdup in romstage
......................................................................

string.h: allow to compile strdup in romstage

Allows to use ramstage compilation units in romstage as long as strdup
isn't called in romstage.

Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed
Signed-off-by: Thomas Heijligen 
---
M src/include/string.h
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/30844/1

diff --git a/src/include/string.h b/src/include/string.h
index fc96393..b692de9 100644
--- a/src/include/string.h
+++ b/src/include/string.h
@@ -51,7 +51,7 @@
    return 0;
 }

-#if !defined(__PRE_RAM__)
+//#if !defined(__PRE_RAM__)
 static inline char *strdup(const char *s)
 {
    size_t sz = strlen(s) + 1;
@@ -69,7 +69,7 @@
    memcpy(d + sz_1, s2, sz_2 + 1);
    return d;
 }
-#endif
+//#endif

/**
  * Find a character in a string.

[Angel Pons (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/30844/1/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/30844/1/src/include/string.h@54 
PS1, Line 54: //#if !defined(__PRE_RAM__)
If this has to be merged, please do not leave this line like this.

[Julius Werner (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/30844/1/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/30844/1/src/include/string.h@56 
PS1, Line 56: {
You can put a

#if !defined(__PRE_RAM__)
  dead_code("Cannot strdup() in pre-RAM environments")
#endif

here, then you'll guarantee with a link-time assertion that calls to this will get optimized out. (I guess the undefined reference to malloc would do the same, but this makes the intent more explicit.)

[Thomas Heijligen (Code Review)]

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30844 )

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 1:
> Patch Set 1:
>
> (1 comment)

When trying to use strdup in romstage you get a linking error:
...: In function `strdup':
.../string.h:58: undefind reference to `malloc'
...: Error 1

[Thomas Heijligen (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

string.h: allow to compile strdup in romstage

Allows to use ramstage compilation units in romstage as long as strdup
isn't called in romstage.

Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed
Signed-off-by: Thomas Heijligen 
---
M src/include/string.h
1 file changed, 0 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/30844/2

[Thomas Heijligen (Code Review)]

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30844 )

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/30844/1/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/#/c/30844/1/src/include/string.h@54 
PS1, Line 54: //#if !defined(__PRE_RAM__)
> If this has to be merged, please do not leave this line like this.

Done

https://review.coreboot.org/#/c/30844/1/src/include/string.h@56 
PS1, Line 56: {
> You can put a […]

When trying to use strdup in romstage you get an usefull linking error:
...: In function `strdup':
.../string.h:58: undefind reference to `malloc'
...: Error 1

[Angel Pons (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4: Code-Review+2

[Stefan Reinauer (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4:

What's the purpose of this? It would be cleaner to seperate code into two files, one for ramstage and one for romstage (and one combined, if that exists) instead.

[Nico Huber (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4:
> Patch Set 4:
>
> What's the purpose of this? It would be cleaner to seperate code into two files, one for ramstage and one for romstage (and one combined, if that exists) instead.

The purpose is to have rom/ramstage code in a common compilation
unit in `qemu-i440fx/fw_cfg.c`. See top of this branch. If you split
that, you need an API between the compilation units (I'm not sure if
the current API in `fw_cfg.h` suffices). It seemed like a little much
overhead so I advised Thomas to remove the guard from the declarations.

Please have a look at the comments here:
https://review.coreboot.org/c/coreboot/+/30844/1/src/include/string.h#56
Would you prefer the explicit error message over the linker error?
Both point to a problem with strdup(), but the explicit message might
protect people from trying to fix it :)

Now that I think about it, the current error (undeclared strdup()) might
cause the most irritation, because that might look like something to fix
for somebody less experienced with coreboot.

[Paul Menzel (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4: Code-Review+1
> Patch Set 4:
> > Patch Set 4:
> >
> > What's the purpose of this? It would be cleaner to seperate code into two files, one for ramstage and one for romstage (and one combined, if that exists) instead.
>
> The purpose is to have rom/ramstage code in a common compilation
> unit in `qemu-i440fx/fw_cfg.c`. See top of this branch. If you split
> that, you need an API between the compilation units (I'm not sure if
> the current API in `fw_cfg.h` suffices). It seemed like a little much
> overhead so I advised Thomas to remove the guard from the declarations.
>
> Please have a look at the comments here:
> https://review.coreboot.org/c/coreboot/+/30844/1/src/include/string.h#56
> Would you prefer the explicit error message over the linker error?
> Both point to a problem with strdup(), but the explicit message might
> protect people from trying to fix it :)
>
> Now that I think about it, the current error (undeclared strdup()) might
> cause the most irritation, because that might look like something to fix
> for somebody less experienced with coreboot.

If I am understanding properly, the issue with strdup() in romstage is that it uses malloc(), and since we're running on very tight memory, dynamic memory is not allowed.
I would prefer to have an explicit message signalling using disallowed functions in romstage is the programmer's fault, and not a compilation error.

[Julius Werner (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4:

Again, I wanna plug using the dead_code() macro for this. It would lead to a message like:

src/include/assert.h:57:2: error: call to 'dead_code_assertion_failed_57' declared with attribute error: "strdup() must not be used in romstage!" in src/include/string.h:24

when you try to build it the wrong way, and it would also make it clear to anyone reading the strdup() code that this is intentional.

[Angel Pons (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 4:
> Patch Set 4:
>
> Again, I wanna plug using the dead_code() macro for this. It would lead to a message like:
>
> src/include/assert.h:57:2: error: call to 'dead_code_assertion_failed_57' declared with attribute error: "strdup() must not be used in romstage!" in src/include/string.h:24
>
> when you try to build it the wrong way, and it would also make it clear to anyone reading the strdup() code that this is intentional.

Definitely agree.

[Thomas Heijligen (Code Review)]

Hello Angel Pons, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

string.h: allow to compile strdup in romstage

Allows to use ramstage compilation units in romstage as long as strdup
isn't called in romstage.

Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed
Signed-off-by: Thomas Heijligen 
---
M src/include/string.h
1 file changed, 7 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/30844/5

[Thomas Heijligen (Code Review)]

Hello Angel Pons, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

string.h: allow to compile strdup in romstage

Allows to use ramstage compilation units in romstage as long as strdup
isn't called in romstage.

Change-Id: I85738a7c7c2f4b0be9b1624b34c478ecdfbb25ed
Signed-off-by: Thomas Heijligen 
---
M src/include/string.h
1 file changed, 0 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/30844/6

[Nico Huber (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 5:
> https://qa.coreboot.org/job/coreboot-gerrit/87667/ : UNSTABLE

It seems everyone has its own ASSERT() defined. I see two ways
to fix it (both not in the scope of this change).

1. don't define ASSERT() in assert.h (e.g. rename it to _ASSERT()). existing calls should call the standard assert()
2. move dead_code() into its own header file

[Julius Werner (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 6:
> don't define ASSERT() in assert.h (e.g. rename it to _ASSERT()). existing calls should call the standard assert()

I think that wouldn't be a bad change (we should standardize on assert() and have nothing else), but I think other files in coreboot shouldn't make up their own ASSERT() either.

Note that that's only half the issues, there's also https://qa.coreboot.org/job/coreboot-gerrit/87667/testReport/junit/board/chr... which I'm not quite sure about. It seems to be some weird include path problem with vboot (and maybe a hint that we shouldn't implement all our string functions as static inlines in headers anyway... why are we doing that?).

[Nico Huber (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 6:
> > don't define ASSERT() in assert.h (e.g. rename it to _ASSERT()). existing calls should call the standard assert()
>
> I think that wouldn't be a bad change (we should standardize on assert() and have nothing else), but I think other files in coreboot shouldn't make up their own ASSERT() either.

Ok, let's keep that in mind. I guess, we don't need it here,
see below.
> Note that that's only half the issues, there's also https://qa.coreboot.org/job/coreboot-gerrit/87667/testReport/junit/board/chr... which I'm not quite sure about. It seems to be some weird include path problem with vboot (and maybe a hint that we shouldn't implement all our string functions as static inlines in headers anyway... why are we doing that?).

So strategy from here, move the bigger implementations into
their own compilation unit? I wouldn't mind keeping some of
the lighter functions inline. But would we then add the new
compilation unit to romstage just for the `dead_code` mes-
sage?

[Julius Werner (Code Review)]

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

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Patch Set 6:
> So strategy from here, move the bigger implementations into
> their own compilation unit? I wouldn't mind keeping some of
> the lighter functions inline. But would we then add the new
> compilation unit to romstage just for the `dead_code` mes-
> sage?

I'd say that's probably a good idea to get more in line with what other codebases (e.g. libpayload) do. I think these string functions aren't actually *that* short or that important that they deserve special treatment over all the other short functions that we don't inline. If you're really worried about performance for those, the better thing to do would probably be to transform them into real functions and provide faster assembly versions. Linking a .c version into romstage shouldn't be a problem because if you don't call strdup(), the whole function will still be garbage-collected out (but if you do call it, you still get the nicer compile-time error from dead_code()).

But that's not really related to this patch... I found the issue that's causing the include problems. Fix at CB:31084.

[Thomas Heijligen (Code Review)]

Thomas Heijligen has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30844 )

Change subject: string.h: allow to compile strdup in romstage
......................................................................

Abandoned

doesn't need anymore