| Message ID | 20190213215728.21603-1-thomas.petazzoni@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] lib/crc16: use non-C99 loop style | expand |
> On 13.02.2019, at 22:57, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Commit 51c2345bd24837f9f67f16268da6dc71573f1325 ("Roll CRC16-CCITT > into the hash infrastructure") has modified the crc16 code by adding a > C99-style loop where the loop iterator is declared inside the for() > statement. This breaks the build with old compiler such as gcc 4.7, > that do not default to C99: I thought U-Boot now has a requirement of GCC6 or newer (i.e. C99 being the default C dialect). While there’s still a few distributions out there that use legacy compiler versions, we might want to consider having the same requirement for the host tools. > ./tools/../lib/crc16.c: In function 'crc16_ccitt': > ./tools/../lib/crc16.c:70:2: error: 'for' loop initial declarations are only allowed in C99 mode > ./tools/../lib/crc16.c:70:2: note: use option -std=c99 or -std=gnu99 to compile your code > > Switching to the regular coding style used in the rest of U-Boot > allows to fix this build issue. At this point in time (although Tom will have the final word), I would strongly encourage us adopting C99 in our coding style. C99 has been around for a while now (it will be 20 years this year that the standard came out) and we already have seen ISO/IEC 9899:2011 and ISO/IEC 9899:2018 published. Note that these also improvements for system-level development (stdatomics.h and stdalign.h are underutilised extensions for that purpose)... > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > lib/crc16.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/lib/crc16.c b/lib/crc16.c > index f46ba727c9..89d2cff131 100644 > --- a/lib/crc16.c > +++ b/lib/crc16.c > @@ -67,7 +67,9 @@ static const uint16_t crc16_tab[] = { > > uint16_t crc16_ccitt(uint16_t cksum, const unsigned char *buf, int len) If we are trying to turn back the wheel of time, then even the const modifier would be up for discussion: after all, this was a GNU extension in gnu89… > { > - for (int i = 0; i < len; i++) > + int i; > + > + for (i = 0; i < len; i++) > cksum = crc16_tab[((cksum>>8) ^ *buf++) & 0xff] ^ (cksum << 8); > > return cksum; > -- > 2.20.1 >
On Wed, Feb 13, 2019 at 10:57:28PM +0100, Thomas Petazzoni wrote: > Commit 51c2345bd24837f9f67f16268da6dc71573f1325 ("Roll CRC16-CCITT > into the hash infrastructure") has modified the crc16 code by adding a > C99-style loop where the loop iterator is declared inside the for() > statement. This breaks the build with old compiler such as gcc 4.7, > that do not default to C99: > > ./tools/../lib/crc16.c: In function 'crc16_ccitt': > ./tools/../lib/crc16.c:70:2: error: 'for' loop initial declarations are only allowed in C99 mode > ./tools/../lib/crc16.c:70:2: note: use option -std=c99 or -std=gnu99 to compile your code > > Switching to the regular coding style used in the rest of U-Boot > allows to fix this build issue. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> So first, as Philipp notes we require gcc-6 or later for U-Boot itself. But you're hitting this on the host and I assume something uncommon but not unsupported yet where it's still on gcc-4.7. So I'm inclined to allow the patch and just note that we're likely to have other breakage in the future. Also, can you please v2 and reword with a Fixes tag instead? Thanks!
Tom, > On 14.02.2019, at 01:31, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Feb 13, 2019 at 10:57:28PM +0100, Thomas Petazzoni wrote: > >> Commit 51c2345bd24837f9f67f16268da6dc71573f1325 ("Roll CRC16-CCITT >> into the hash infrastructure") has modified the crc16 code by adding a >> C99-style loop where the loop iterator is declared inside the for() >> statement. This breaks the build with old compiler such as gcc 4.7, >> that do not default to C99: >> >> ./tools/../lib/crc16.c: In function 'crc16_ccitt': >> ./tools/../lib/crc16.c:70:2: error: 'for' loop initial declarations are only allowed in C99 mode >> ./tools/../lib/crc16.c:70:2: note: use option -std=c99 or -std=gnu99 to compile your code >> >> Switching to the regular coding style used in the rest of U-Boot >> allows to fix this build issue. >> >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > So first, as Philipp notes we require gcc-6 or later for U-Boot itself. > But you're hitting this on the host and I assume something uncommon but > not unsupported yet where it's still on gcc-4.7. So I'm inclined to > allow the patch and just note that we're likely to have other breakage > in the future. Also, can you please v2 and reword with a Fixes tag > instead? Thanks! At the moment, our code requires at least GNU89 (i.e. not C89) or C99, even when compiling our host tools (which shouldn’t require any GNU extensions, as we shouldn’t need inline-asm in the host tools). So the earliest ISO standard language dialect we can hope to comply with is ISO/IEC 9899:1999 (i.e. C99). I’ve never been a big fan of requiring GNU89 (i.e. GCC) just to get some of the C99 features we need (e.g. offsetof, const, …) when the source can simply be declared to be C99. So shouldn’t we just add a “-std=c99” to HOST_EXTRACFLAGS? Note that all of the ancient GCC versions still shipped with distributions (you don’t want to know how many hours I’ve spend supporting issues caused by CentOS still being on gcc-4.8 even on AArch64) will support C99: C99 support was completed in time for gcc-4.5 (AFAIR). --Phil. p.s.: I’m sorry for harping on about this, but I have become a bit of language lawyer over the years… and I promise to shut up about this specific occurance after this email ;-)
Hello, On Thu, 14 Feb 2019 01:57:02 +0100 Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > At the moment, our code requires at least GNU89 (i.e. not C89) or C99, even > when compiling our host tools (which shouldn’t require any GNU extensions, > as we shouldn’t need inline-asm in the host tools). > So the earliest ISO standard language dialect we can hope to comply with is > ISO/IEC 9899:1999 (i.e. C99). > > I’ve never been a big fan of requiring GNU89 (i.e. GCC) just to get some of > the C99 features we need (e.g. offsetof, const, …) when the source can simply > be declared to be C99. > > So shouldn’t we just add a “-std=c99” to HOST_EXTRACFLAGS? Adding -std=c99 to the CFLAGS used when building host tools would indeed be another solution, which would work equally well for me. However, generally speaking is U-Boot interested in allowing this kind of C99 variable declaration ? For example, the Linux kernel coding style doesn't allow this, but perhaps U-Boot has made a difference choice here. I don't have any strong opinion about this: we just bumped to U-Boot 2019.01 in Buildroot, and one of our autobuilders that intentionally uses a very old Debian system encountered this build failure. Buildroot is used in lots of "enterprise" contexts, where enterprise often means "the IT forces the poor developers to use antique Linux systems for their job", and we're trying to make the life of those poor developers slightly easier :-) Best regards, Thomas
Tom & Thomas, > On 14.02.2019, at 08:56, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > However, generally speaking is U-Boot interested in allowing this kind > of C99 variable declaration ? For example, the Linux kernel coding > style doesn't allow this, but perhaps U-Boot has made a difference > choice here. I took the opportunity to see how widespread the use of this style already is in U-Boot (as it will eventually creep in, now that we’ve moved to GCC6 or newer) and there’s only a few occurences to date: board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) board/synopsys/hsdk/hsdk.c: for (u32 i = MASTER_CPU_ID + 1; i < NR_CPUS; i++) board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { lib/crc16.c: for (int i = 0; i < len; i++) Thanks, Philipp.
On Thu, Feb 14, 2019 at 12:35:12PM +0100, Philipp Tomsich wrote: > Tom & Thomas, > > > On 14.02.2019, at 08:56, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > > > However, generally speaking is U-Boot interested in allowing this kind > > of C99 variable declaration ? For example, the Linux kernel coding > > style doesn't allow this, but perhaps U-Boot has made a difference > > choice here. > > > I took the opportunity to see how widespread the use of this style already > is in U-Boot (as it will eventually creep in, now that we’ve moved to GCC6 > or newer) and there’s only a few occurences to date: > > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { > board/synopsys/hsdk/env-lib.c: for (u32 i = 0; map[i].env_name; i++) { > board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { > board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) > board/synopsys/hsdk/hsdk.c: for (u32 i = MASTER_CPU_ID + 1; i < NR_CPUS; i++) > board/synopsys/hsdk/hsdk.c: for (u32 i = 0; i < NR_CPUS; i++) { > lib/crc16.c: for (int i = 0; i < len; i++) Well, lets go with the C99 flag instead for v2, thanks again!
diff --git a/lib/crc16.c b/lib/crc16.c index f46ba727c9..89d2cff131 100644 --- a/lib/crc16.c +++ b/lib/crc16.c @@ -67,7 +67,9 @@ static const uint16_t crc16_tab[] = { uint16_t crc16_ccitt(uint16_t cksum, const unsigned char *buf, int len) { - for (int i = 0; i < len; i++) + int i; + + for (i = 0; i < len; i++) cksum = crc16_tab[((cksum>>8) ^ *buf++) & 0xff] ^ (cksum << 8); return cksum;
Commit 51c2345bd24837f9f67f16268da6dc71573f1325 ("Roll CRC16-CCITT into the hash infrastructure") has modified the crc16 code by adding a C99-style loop where the loop iterator is declared inside the for() statement. This breaks the build with old compiler such as gcc 4.7, that do not default to C99: ./tools/../lib/crc16.c: In function 'crc16_ccitt': ./tools/../lib/crc16.c:70:2: error: 'for' loop initial declarations are only allowed in C99 mode ./tools/../lib/crc16.c:70:2: note: use option -std=c99 or -std=gnu99 to compile your code Switching to the regular coding style used in the rest of U-Boot allows to fix this build issue. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- lib/crc16.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)