Attention is currently required from: Jakub Czapiga, Yu-Ping Wu.
Julius Werner has posted comments on this change by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/83765?usp=email )
Change subject: commonlib/bsd: Add strncat() and strcat() functions
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
File src/lib/string.c:
https://review.coreboot.org/c/coreboot/+/83765/comment/80ef7441_b6dd47ed?us… :
PS4, Line 119: strcpy(dst + strlen(dst), src);
> Because `strcpy` references `strlen`, I moved `strlen` to commonlib/bsd in CB:83830. […]
Yeah, sorry, you're right. I was completely off-track here and didn't think about the problem closely enough.
There are many string functions where calling `strlen()` internally creates performance issues (some particularly bad ones in libpayload, e.g. `strcpy()` and `strncpy()`), but this isn't one of them.
--
To view, visit https://review.coreboot.org/c/coreboot/+/83765?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: If02fce0eafb4f6fa01d8bab17d87a32360f4ac83
Gerrit-Change-Number: 83765
Gerrit-PatchSet: 5
Gerrit-Owner: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Thu, 08 Aug 2024 20:19:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Julius Werner <jwerner(a)chromium.org>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Attention is currently required from: Arthur Heymans.
Nico Huber has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/83559?usp=email )
Change subject: Makefile.mk: Remove linker warning on RWX segments
......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
> I've rebased the patch. Please review.
Done
--
To view, visit https://review.coreboot.org/c/coreboot/+/83559?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1e0f51c69dabaea314ac45924474d446a9ab68f4
Gerrit-Change-Number: 83559
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Felix Held <felix-coreboot(a)felixheld.de>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-CC: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Comment-Date: Thu, 08 Aug 2024 20:17:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Comment-In-Reply-To: Arthur Heymans <arthur(a)aheymans.xyz>
Comment-In-Reply-To: Yu-Ping Wu <yupingso(a)google.com>
Attention is currently required from: Jakub Czapiga, Maximilian Brune, Yu-Ping Wu.
Julius Werner has posted comments on this change by Yu-Ping Wu. ( https://review.coreboot.org/c/coreboot/+/83830?usp=email )
Change subject: commonlib/bsd: Add strnlen() and strlen() functions
......................................................................
Patch Set 1: Code-Review+2
(3 comments)
File payloads/libpayload/libc/string.c:
https://review.coreboot.org/c/coreboot/+/83830/comment/de0c20d8_f6aeeb3d?us… :
PS1, Line 71: /* NULL and empty strings have length 0. */
: if (!str)
: return 0;
> You don't have this NULL check in the commonlib implementation. […]
There's no NULL check required here by POSIX and other major libc implementations (e.g. glibc: https://github.com/bminor/glibc/blob/master/sysdeps/i386/strlen.c) don't seem to do that. I see no reason why libpayload should do something completely out of turn here, and this seems like as good an opportunity as any to fix that. (It's arguable whether returning 0 is even "better" here. If a caller calls `strlen()` on `NULL` that's always a bug anyway and just silently returning a result that's as incorrect as any other doesn't necessarily make things better.)
File src/commonlib/bsd/string.c:
https://review.coreboot.org/c/coreboot/+/83830/comment/b4e3a6b5_f9174799?us… :
PS1, Line 12: return len;
nit: This is probably overthinking it, but I get a shorter loop in assembly with
```
const char *ptr = 0;
while (*ptr++)
/* walk string */;
return ptr - str - 1;
```
https://review.coreboot.org/c/coreboot/+/83830/comment/194d8b21_563d6938?us… :
PS1, Line 20: return len;
Here it would be
```
const char *ptr = str;
const char *end = str + n + 1;
while (*ptr++ && ptr < end)
/* walk string */;
return ptr - str - 1;
```
(I know that makes readability suffer a bit so maybe it's not a good idea. But in general, anything that it can rewrite to check a condition at the end tends to save a jump in the loop. It also seems be able to fit in the post-increment better when it happens at the start (especially on AArch64), and there's no separate counter it could use to calculate offsets instead.)
--
To view, visit https://review.coreboot.org/c/coreboot/+/83830?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1203ec9affabe493bd14b46662d212b08240cced
Gerrit-Change-Number: 83830
Gerrit-PatchSet: 1
Gerrit-Owner: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Reviewer: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Jakub Czapiga <czapiga(a)google.com>
Gerrit-Attention: Maximilian Brune <maximilian.brune(a)9elements.com>
Gerrit-Attention: Yu-Ping Wu <yupingso(a)google.com>
Gerrit-Comment-Date: Thu, 08 Aug 2024 20:16:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Maximilian Brune <maximilian.brune(a)9elements.com>
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/83693?usp=email )
Change subject: Makefile: Move `--no-warn-rwx-segments' into xcompile
......................................................................
Makefile: Move `--no-warn-rwx-segments' into xcompile
The parameter is not available for binutils older than 2.39. So move it
to xcompile to provide backwards compatibility for a bit.
Change-Id: I02982769ae2c356f037a747e85d155368bfcb730
Signed-off-by: Nico Huber <nico.h(a)gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/83693
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
---
M Makefile.mk
M util/xcompile/xcompile
2 files changed, 6 insertions(+), 3 deletions(-)
Approvals:
build bot (Jenkins): Verified
Felix Singer: Looks good to me, approved
Nico Huber: Looks good to me, approved
diff --git a/Makefile.mk b/Makefile.mk
index e9ad2cc..901d6b2 100644
--- a/Makefile.mk
+++ b/Makefile.mk
@@ -604,9 +604,6 @@
LDFLAGS_common += --nmagic
LDFLAGS_common += -static
LDFLAGS_common += -z noexecstack
-# Disable warning on segments with RWX.
-# All loadable sections are placed in the same segment for simplicity.
-LDFLAGS_common += --no-warn-rwx-segments
# Workaround for RISC-V linker bug, merge back into above line when fixed.
# https://sourceware.org/bugzilla/show_bug.cgi?id=27180
diff --git a/util/xcompile/xcompile b/util/xcompile/xcompile
index 4ead648..13ed48b 100755
--- a/util/xcompile/xcompile
+++ b/util/xcompile/xcompile
@@ -241,6 +241,12 @@
testcc "$GCC" "$CFLAGS_GCC -Wextra" &&
CFLAGS_GCC="$CFLAGS_GCC -Wextra"
+ # Disable warning on segments with RWX.
+ # All loadable sections are placed in the same segment for simplicity.
+ testld "$GCC" "$FLAGS_GCC" "${GCCPREFIX}ld${LINKER_SUFFIX}" \
+ "$LDFLAGS --no-warn-rwx-segments" && \
+ LDFLAGS="$LDFLAGS --no-warn-rwx-segments"
+
case "$architecture" in
x86)
;;
--
To view, visit https://review.coreboot.org/c/coreboot/+/83693?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02982769ae2c356f037a747e85d155368bfcb730
Gerrit-Change-Number: 83693
Gerrit-PatchSet: 4
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Arthur Heymans, Elyes Haouas, Felix Singer, Matt DeVillier.
Nico Huber has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/83693?usp=email )
Change subject: Makefile: Move `--no-warn-rwx-segments' into xcompile
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Patchset:
PS3:
Thanks
--
To view, visit https://review.coreboot.org/c/coreboot/+/83693?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I02982769ae2c356f037a747e85d155368bfcb730
Gerrit-Change-Number: 83693
Gerrit-PatchSet: 3
Gerrit-Owner: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Reviewer: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <service+coreboot-gerrit(a)felixsinger.de>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Attention: Elyes Haouas <ehaouas(a)noos.fr>
Gerrit-Comment-Date: Thu, 08 Aug 2024 20:16:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes