[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Thomas Petazzoni]

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(-)

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;
-- 
2.20.1

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Philipp Tomsich]

> 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
> 

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Tom Rini]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190213/a9344742/attachment.sig>

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Philipp Tomsich]

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 ;-)

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Philipp Tomsich]

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.

[U-Boot] [PATCH] lib/crc16: use non-C99 loop style

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190214/23511b55/attachment.sig>