Hello Alex Levin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42234
to review the following change.
Change subject: libpayload: Fix potential compilation issues ......................................................................
libpayload: Fix potential compilation issues
The include chain can define one of the marcros (BIT, MIN, MAX ar all very common) and this will cause a compilation issue. Wrapping with ifndef to protect from that.
Change-Id: I1e1c1f8e75b752bc3d78e0c843866d0144b82b2a Signed-off-by: Alex Levin levinale@chromium.org --- M payloads/libpayload/include/libpayload.h 1 file changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42234/1
diff --git a/payloads/libpayload/include/libpayload.h b/payloads/libpayload/include/libpayload.h index 25080d8..5b9f052 100644 --- a/payloads/libpayload/include/libpayload.h +++ b/payloads/libpayload/include/libpayload.h @@ -66,31 +66,55 @@ #include <archive.h>
/* Double-evaluation unsafe min/max, for bitfields and outside of functions */ +#ifndef __CMP_UNSAFE #define __CMP_UNSAFE(a, b, op) ((a) op (b) ? (a) : (b)) -#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) -#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) +#endif
+#ifndef MIN_UNSAFE +#define MIN_UNSAFE(a, b) __CMP_UNSAFE(a, b, <) +#endif + +#ifndef MAX_UNSAFE +#define MAX_UNSAFE(a, b) __CMP_UNSAFE(a, b, >) +#endif + +#ifndef __CMP_SAFE #define __CMP_SAFE(a, b, op, var_a, var_b) ({ \ __TYPEOF_UNLESS_CONST(a, b) var_a = (a); \ __TYPEOF_UNLESS_CONST(b, a) var_b = (b); \ var_a op var_b ? var_a : var_b; \ }) +#endif
+#ifndef __CMP #define __CMP(a, b, op) __builtin_choose_expr( \ __builtin_constant_p(a) && __builtin_constant_p(b), \ __CMP_UNSAFE(a, b, op), __CMP_SAFE(a, b, op, __TMPNAME, __TMPNAME)) +#endif
+#ifndef MIN #define MIN(a, b) __CMP(a, b, <) +#endif + +#ifndef MAX #define MAX(a, b) __CMP(a, b, >) +#endif
+#ifndef ARRAY_SIZE #define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0])) -#define BIT(x) (1ul << (x)) +#endif
+#ifndef BIT +#define BIT(x) (1ul << (x)) +#endif + +#ifndef DIV_ROUND_UP #define DIV_ROUND_UP(x, y) ({ \ typeof(x) _div_local_x = (x); \ typeof(y) _div_local_y = (y); \ (_div_local_x + _div_local_y - 1) / _div_local_y; \ }) +#endif
static inline u32 div_round_up(u32 n, u32 d) { return (n + d - 1) / d; }
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG@9 PS1, Line 9: ar "are" ?
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG@9 PS1, Line 9: marcros macros
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG@9 PS1, Line 9: ar
"are" ?
Ack
https://review.coreboot.org/c/coreboot/+/42234/1//COMMIT_MSG@9 PS1, Line 9: marcros
macros
Ack
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Nick Vaccaro, Alex Levin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42234
to look at the new patch set (#2).
Change subject: libpayload: Fix potential compilation issues ......................................................................
libpayload: Fix potential compilation issues
The include chain can define one of the macros (BIT, MIN, MAX are all very common) and this will cause a compilation issue. Wrapping with ifndef to protect from that.
Change-Id: I1e1c1f8e75b752bc3d78e0c843866d0144b82b2a Signed-off-by: Alex Levin levinale@chromium.org --- M payloads/libpayload/include/libpayload.h 1 file changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/34/42234/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all They really shouldn't, though. I'm okay putting this in if it helps building some rogue payloads that just throw code together without really caring what's in there, but if this happens in libpayload code itself (or in depthcharge), please consider that a bug and fix it. We should not have duplicate (or worse, diverging) definitions of the same thing.
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all
They really shouldn't, though. […]
in some scenarios, this h file will be included (not directly) with others (e.g. ec_commands.h) in which case BIT is redefined. Generally, defining marcos in h files should be done in a "Safe" matter so to speak.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all
in some scenarios, this h file will be included (not directly) with others (e.g. ec_commands. […]
No, I disagree. Macros like these are APIs just like functions, and they should be uniquely defined with a single implementation across this project. Header ordering should never have an effect on how the code works, but by doing this you risk exactly that, because which of the two definitions you're pulling in depends only on ordering (and you won't notice if they diverge). Macro definitions should not be conditionalized like this except for special circumstances.
ec_commands.h is special because it gets pulled in from another project with different definitions for these macros. That's why ec_commands.h itself has #ifndefs like this. The appropriate solution there is to make sure that ec_commands.h is always included after libpayload.h, which you can cleanly achieve by creating a wrapper header that includes both files and making sure all other depthcharge files always include that wrapper header and never ec_commands.h directly. This issue was already discussed in CL:2126525, I don't know why nothing came out of it in the end.
If this is your only problem, please solve it by including "src/drivers/ec/cros/commands.h" from your depthcharge code (instead of <ec_commands.h> directly) and adding an #include <libpayload.h> to the top of that header.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2:
+Caveh, do you remember what happened to the <ec_commands.h> wrapper discussion (see CL:2126525)? Did we end up doing something else or did we just forget?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2: -Code-Review
Patch Set 2:
+Caveh, do you remember what happened to the <ec_commands.h> wrapper discussion (see CL:2126525)? Did we end up doing something else or did we just forget?
Well, since we're using ec_commands.h in so many projects, maybe we should reconsider the dependencies it introduces, such as namespace collisons. I think either a prefix or removing macros like that from the file may be more appropriate
Alex Levin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all
No, I disagree. […]
I still think this is the better, more robust approach (and in the same time fix every time it's misused).
Possibly the better approach here is a warning?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42234/2//COMMIT_MSG@9 PS2, Line 9: The include chain can define one of the macros (BIT, MIN, MAX are all
I still think this is the better, more robust approach (and in the same time fix every time it's mis […]
I'm okay with putting this in with a #warning directive if you want to, that may be useful for other payloads which don't care about doing things the right way as much. Note that for depthcharge, all warnings are treated as errors, so it would still prevent compilation there (as I think it should).
Alex Levin has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42234 )
Change subject: libpayload: Fix potential compilation issues ......................................................................
Abandoned
-
Alex Levin (Code Review) -
Julius Werner (Code Review)
-
Nick Vaccaro (Code Review)
-
Tim Wawrzynczak (Code Review)