HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
(test)lz4/lib: use 'std=c17'
Change-Id: Ib7f464f31198155feb2a7c249cc2a0a8ab60e3ba Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M util/cbfstool/lz4/lib/Makefile 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/44194/1
diff --git a/util/cbfstool/lz4/lib/Makefile b/util/cbfstool/lz4/lib/Makefile index 12a741d..28e6963 100644 --- a/util/cbfstool/lz4/lib/Makefile +++ b/util/cbfstool/lz4/lib/Makefile @@ -41,7 +41,7 @@ PREFIX ?= /usr/local CPPFLAGS= -DXXH_NAMESPACE=LZ4_ CFLAGS ?= -O3 -CFLAGS += -std=c99 -Wall -Wextra -Wundef -Wshadow -Wcast-align -Wcast-qual -Wstrict-prototypes -pedantic +CFLAGS += -std=c17 -Wall -Wextra -Wundef -Wshadow -Wcast-align -Wcast-qual -Wstrict-prototypes -pedantic FLAGS = $(CPPFLAGS) $(CFLAGS) $(LDFLAGS)
LIBDIR?= $(PREFIX)/lib
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1: Code-Review-1
This code is imported verbatim from LZ4's GitHub, so let's not touch it unless necessary.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1: Code-Review-1
Patch Set 1: Code-Review-1
This code is imported verbatim from LZ4's GitHub, so let's not touch it unless necessary.
I've checked, at GitHub, they don't even use 'std='. So by default, GCC will use c17.
Also, it seems, that we are using an old version.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1: Code-Review-1
Patch Set 1: Code-Review-1
Patch Set 1: Code-Review-1
This code is imported verbatim from LZ4's GitHub, so let's not touch it unless necessary.
I've checked, at GitHub, they don't even use 'std='. So by default, GCC will use c17.
Also, it seems, that we are using an old version.
AFAIUI, GCC by default uses *gnu17* according to the documentation: https://gcc.gnu.org/onlinedocs/gcc/Standards.html
The default, if no C language dialect options are given, is -std=gnu17.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1:
This code is imported verbatim from LZ4's GitHub, so let's not touch it unless necessary.
I've checked, at GitHub, they don't even use 'std='. So by default, GCC will use c17.
Hmm... looks like I unfortunately didn't write down which version exactly we pulled in, which I probably should have. It looks like it may have been a fork at the time anyway because we needed an extra patch for in-place decoding support that was only accepted upstream in 2016 (https://github.com/lz4/lz4/commit/2995a45e538d6a0741c3300d4b06b871eed72247), but lz4 landed in coreboot in 2015 already.
Anyway, judging from the timestamps it was probably based on a version somewhere around https://github.com/lz4/lz4/commit/012ab2f5297bec68602ce1f4b5739beb5b99673e, which as you can see had that -std=c99 flag: https://github.com/lz4/lz4/blob/012ab2f5297bec68602ce1f4b5739beb5b99673e/lib...
Also, it seems, that we are using an old version.
Yes, it hasn't been touched since -- and that's the idea. We don't want to be in the business of maintaining an LZ4 fork. We know this code works, we put a bunch of effort into making sure of that when we first imported it and it has been tested and proven faultless a lot since. This is the kind of code that's so isolated and platform-independent it doesn't just stop working on its own at some point. But it is also extremely complicated stuff (and not super readable tbh) so I would rather just not touch it at all as much as possible instead of risking to introduce new issues.
We could uprev to a newer version but that is a larger effort... we should be uprevving everything at once (the cbfstool stuff here and the actual coreboot version in commonlib), and then we should be benchmarking it to make sure speed and binary size are actually better than our current version for our use cases, and to make sure it still works in all cases. For example, the current decompression function looks vastly different than what we have and I don't see any trace of my in-place decompression support feature in there anymore... since we're probably the only ones who care about that I'm afraid it may have been dropped upstream at some point, so someone would have to do that due diligence of really digging through the new algorithm version and making sure it can still catch output->input overruns in all cases.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1:
Patch Set 1:
This code is imported verbatim from LZ4's GitHub, so let's not touch it unless necessary.
I've checked, at GitHub, they don't even use 'std='. So by default, GCC will use c17.
Hmm... looks like I unfortunately didn't write down which version exactly we pulled in, which I probably should have. It looks like it may have been a fork at the time anyway because we needed an extra patch for in-place decoding support that was only accepted upstream in 2016 (https://github.com/lz4/lz4/commit/2995a45e538d6a0741c3300d4b06b871eed72247), but lz4 landed in coreboot in 2015 already.
Anyway, judging from the timestamps it was probably based on a version somewhere around https://github.com/lz4/lz4/commit/012ab2f5297bec68602ce1f4b5739beb5b99673e, which as you can see had that -std=c99 flag: https://github.com/lz4/lz4/blob/012ab2f5297bec68602ce1f4b5739beb5b99673e/lib...
Also, it seems, that we are using an old version.
Yes, it hasn't been touched since -- and that's the idea. We don't want to be in the business of maintaining an LZ4 fork. We know this code works, we put a bunch of effort into making sure of that when we first imported it and it has been tested and proven faultless a lot since. This is the kind of code that's so isolated and platform-independent it doesn't just stop working on its own at some point. But it is also extremely complicated stuff (and not super readable tbh) so I would rather just not touch it at all as much as possible instead of risking to introduce new issues.
We could uprev to a newer version but that is a larger effort... we should be uprevving everything at once (the cbfstool stuff here and the actual coreboot version in commonlib), and then we should be benchmarking it to make sure speed and binary size are actually better than our current version for our use cases, and to make sure it still works in all cases. For example, the current decompression function looks vastly different than what we have and I don't see any trace of my in-place decompression support feature in there anymore... since we're probably the only ones who care about that I'm afraid it may have been dropped upstream at some point, so someone would have to do that due diligence of really digging through the new algorithm version and making sure it can still catch output->input overruns in all cases.
Thank you very much for the explanation.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Patch Set 1:
For the record, it was tried for LZMA [1].
[1]: https://review.coreboot.org/c/coreboot/+/41963
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/44194 )
Change subject: (test)lz4/lib: use 'std=c17' ......................................................................
Abandoned
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
Paul Menzel (Code Review)