[OpenRISC] [PATCH] bfd/elf32-or1k: fix building with gcc version < 5

[OpenRISC] [PATCH] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
an old compiler(i.e. gcc 4.9) build fails on:
```
elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
C99 or C11 mode
    for (size_t i = 0; i < insn_count; i++)
    ^
```

So let's declare `size_t i` at the top of the function instead of inside
for loop.

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
 bfd/elf32-or1k.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/bfd/elf32-or1k.c b/bfd/elf32-or1k.c
index 4ae7f324d33..32063ab0289 100644
--- a/bfd/elf32-or1k.c
+++ b/bfd/elf32-or1k.c
@@ -2244,9 +2244,10 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
 {
   unsigned nodelay = elf_elfheader (output_bfd)->e_flags & EF_OR1K_NODELAY;
   unsigned output_insns[PLT_MAX_INSN_COUNT];
+  size_t i;
 
   /* Copy instructions into the output buffer.  */
-  for (size_t i = 0; i < insn_count; i++)
+  for (i = 0; i < insn_count; i++)
     output_insns[i] = insns[i];
 
   /* Honor the no-delay-slot setting.  */
@@ -2277,7 +2278,7 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
     }
 
   /* Write out the output buffer.  */
-  for (size_t i = 0; i < (insn_count+1); i++)
+  for (i = 0; i < (insn_count+1); i++)
     bfd_put_32 (output_bfd, output_insns[i], contents + (i*4));
 }
 
-- 
2.25.1

[OpenRISC] [PATCH] bfd/elf32-or1k: fix building with gcc version < 5

[Stafford Horne]

On Wed, Jun 09, 2021 at 05:30:59PM +0200, Giulio Benetti wrote:
> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
> an old compiler(i.e. gcc 4.9) build fails on:
> ```
> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
> C99 or C11 mode
>     for (size_t i = 0; i < insn_count; i++)
>     ^
> ```
> 
> So let's declare `size_t i` at the top of the function instead of inside
> for loop.

This looks ok to me.  Can you also include the changelog entry needed for
binutils patches?

Something like:

bfd/ChangeLog:

	* elf32-or1k.c (or1k_write_plt_entry): Move i declaration to top of
	function.

> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> ---
>  bfd/elf32-or1k.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/bfd/elf32-or1k.c b/bfd/elf32-or1k.c
> index 4ae7f324d33..32063ab0289 100644
> --- a/bfd/elf32-or1k.c
> +++ b/bfd/elf32-or1k.c
> @@ -2244,9 +2244,10 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
>  {
>    unsigned nodelay = elf_elfheader (output_bfd)->e_flags & EF_OR1K_NODELAY;
>    unsigned output_insns[PLT_MAX_INSN_COUNT];
> +  size_t i;
>  
>    /* Copy instructions into the output buffer.  */
> -  for (size_t i = 0; i < insn_count; i++)
> +  for (i = 0; i < insn_count; i++)
>      output_insns[i] = insns[i];
>  
>    /* Honor the no-delay-slot setting.  */
> @@ -2277,7 +2278,7 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
>      }
>  
>    /* Write out the output buffer.  */
> -  for (size_t i = 0; i < (insn_count+1); i++)
> +  for (i = 0; i < (insn_count+1); i++)
>      bfd_put_32 (output_bfd, output_insns[i], contents + (i*4));
>  }
>  
> -- 
> 2.25.1
> 

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
an old compiler(i.e. gcc 4.9) build fails on:
```
elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
C99 or C11 mode
    for (size_t i = 0; i < insn_count; i++)
    ^
```

So let's declare `size_t i` at the top of the function instead of inside
for loop.

bfd/ChangeLog:

	* elf32-or1k.c (or1k_write_plt_entry): Move i declaration to top
	of function.

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
 bfd/elf32-or1k.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/bfd/elf32-or1k.c b/bfd/elf32-or1k.c
index 4ae7f324d33..32063ab0289 100644
--- a/bfd/elf32-or1k.c
+++ b/bfd/elf32-or1k.c
@@ -2244,9 +2244,10 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
 {
   unsigned nodelay = elf_elfheader (output_bfd)->e_flags & EF_OR1K_NODELAY;
   unsigned output_insns[PLT_MAX_INSN_COUNT];
+  size_t i;
 
   /* Copy instructions into the output buffer.  */
-  for (size_t i = 0; i < insn_count; i++)
+  for (i = 0; i < insn_count; i++)
     output_insns[i] = insns[i];
 
   /* Honor the no-delay-slot setting.  */
@@ -2277,7 +2278,7 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
     }
 
   /* Write out the output buffer.  */
-  for (size_t i = 0; i < (insn_count+1); i++)
+  for (i = 0; i < (insn_count+1); i++)
     bfd_put_32 (output_bfd, output_insns[i], contents + (i*4));
 }
 
-- 
2.25.1

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Alan Modra]

On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
> an old compiler(i.e. gcc 4.9) build fails on:
> ```
> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
> C99 or C11 mode
>     for (size_t i = 0; i < insn_count; i++)
>     ^
> ```

Did you find this problem on current mainline binutils?  The configure
machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
when using older compilers.  That happens for me when I build with
gcc-4.9.  I don't think any patch is needed for mainline.

-- 
Alan Modra
Australia Development Lab, IBM

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Hello Alan, All,

On 6/10/21 3:19 AM, Alan Modra wrote:
> On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
>> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
>> an old compiler(i.e. gcc 4.9) build fails on:
>> ```
>> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
>> C99 or C11 mode
>>      for (size_t i = 0; i < insn_count; i++)
>>      ^
>> ```
> 
> Did you find this problem on current mainline binutils?  The configure
> machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
> when using older compilers.  That happens for me when I build with
> gcc-4.9.  I don't think any patch is needed for mainline.
> 

On Buildroot they don't pass -std=c99/g99 and this is the result:
https://gitlab.com/bootlin/toolchains-builder/-/jobs/1325646298

This patch seems to follow all the rest code style of binutils since no 
other part of it fails and this happens only after patch [1] has been 
upstreamed.

Here [2] you can see all the other toolchains built succesfully and only 
Openrisc fails after the patch provided by Stafford([1]).

[1]: http://patches-tcwg.linaro.org/patch/53151/
[2]: https://gitlab.com/bootlin/toolchains-builder/-/jobs

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Alan Modra]

On Thu, Jun 10, 2021 at 11:07:13AM +0200, Giulio Benetti wrote:
> Hello Alan, All,
> 
> On 6/10/21 3:19 AM, Alan Modra wrote:
> > On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
> > > Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
> > > an old compiler(i.e. gcc 4.9) build fails on:
> > > ```
> > > elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
> > > C99 or C11 mode
> > >      for (size_t i = 0; i < insn_count; i++)
> > >      ^
> > > ```
> > 
> > Did you find this problem on current mainline binutils?  The configure
> > machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
> > when using older compilers.  That happens for me when I build with
> > gcc-4.9.  I don't think any patch is needed for mainline.
> > 
> 
> On Buildroot they don't pass -std=c99/g99 and this is the result:
> https://gitlab.com/bootlin/toolchains-builder/-/jobs/1325646298

That appears to be building binutils 2.35.2

> This patch seems to follow all the rest code style of binutils

True, we've only just switched over to requiring C99.

> since no
> other part of it fails and this happens only after patch [1] has been
> upstreamed.
> 
> Here [2] you can see all the other toolchains built succesfully and only
> Openrisc fails after the patch provided by Stafford([1]).
> 
> [1]: http://patches-tcwg.linaro.org/patch/53151/
> [2]: https://gitlab.com/bootlin/toolchains-builder/-/jobs

OK, so the real problem is in a backport.  It isn't current mainline
binutils configure, which is what I was worried about.

BTW, thank you for posting a fix here, even if it isn't strictly
necessary for mainline.  Please note that I'm not advocating against
your patch.  If target maintainers want to keep their code compatible
with C89 that's fine by me.

-- 
Alan Modra
Australia Development Lab, IBM

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Hello Alan, All,

On 6/10/21 3:06 PM, Alan Modra wrote:
> On Thu, Jun 10, 2021 at 11:07:13AM +0200, Giulio Benetti wrote:
>> Hello Alan, All,
>>
>> On 6/10/21 3:19 AM, Alan Modra wrote:
>>> On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
>>>> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
>>>> an old compiler(i.e. gcc 4.9) build fails on:
>>>> ```
>>>> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
>>>> C99 or C11 mode
>>>>       for (size_t i = 0; i < insn_count; i++)
>>>>       ^
>>>> ```
>>>
>>> Did you find this problem on current mainline binutils?  The configure
>>> machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
>>> when using older compilers.  That happens for me when I build with
>>> gcc-4.9.  I don't think any patch is needed for mainline.
>>>
>>
>> On Buildroot they don't pass -std=c99/g99 and this is the result:
>> https://gitlab.com/bootlin/toolchains-builder/-/jobs/1325646298
> 
> That appears to be building binutils 2.35.2
> 
>> This patch seems to follow all the rest code style of binutils
> 
> True, we've only just switched over to requiring C99.

Ok so...

>> since no
>> other part of it fails and this happens only after patch [1] has been
>> upstreamed.
>>
>> Here [2] you can see all the other toolchains built succesfully and only
>> Openrisc fails after the patch provided by Stafford([1]).
>>
>> [1]: http://patches-tcwg.linaro.org/patch/53151/
>> [2]: https://gitlab.com/bootlin/toolchains-builder/-/jobs
> 
> OK, so the real problem is in a backport.  It isn't current mainline
> binutils configure, which is what I was worried about.

...it happens on mainline too but it can be solved by adding -std=c99 to 
CFLAGS.

> BTW, thank you for posting a fix here, even if it isn't strictly
> necessary for mainline.  Please note that I'm not advocating against
> your patch.  If target maintainers want to keep their code compatible
> with C89 that's fine by me.
>
I didn't know about this since no other file failed building on C99. 
What it seems strange to me is that on buildroot binutils seem to be 
built without -std=c99

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Stafford Horne]

On Thu, Jun 10, 2021 at 04:48:46PM +0200, Giulio Benetti wrote:
> Hello Alan, All,
> 
> On 6/10/21 3:06 PM, Alan Modra wrote:
> > On Thu, Jun 10, 2021 at 11:07:13AM +0200, Giulio Benetti wrote:
> > > Hello Alan, All,
> > > 
> > > On 6/10/21 3:19 AM, Alan Modra wrote:
> > > > On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
> > > > > Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
> > > > > an old compiler(i.e. gcc 4.9) build fails on:
> > > > > ```
> > > > > elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
> > > > > C99 or C11 mode
> > > > >       for (size_t i = 0; i < insn_count; i++)
> > > > >       ^
> > > > > ```
> > > > 
> > > > Did you find this problem on current mainline binutils?  The configure
> > > > machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
> > > > when using older compilers.  That happens for me when I build with
> > > > gcc-4.9.  I don't think any patch is needed for mainline.
> > > > 
> > > 
> > > On Buildroot they don't pass -std=c99/g99 and this is the result:
> > > https://gitlab.com/bootlin/toolchains-builder/-/jobs/1325646298
> > 
> > That appears to be building binutils 2.35.2
> > 
> > > This patch seems to follow all the rest code style of binutils
> > 
> > True, we've only just switched over to requiring C99.
> 
> Ok so...
> 
> > > since no
> > > other part of it fails and this happens only after patch [1] has been
> > > upstreamed.
> > > 
> > > Here [2] you can see all the other toolchains built succesfully and only
> > > Openrisc fails after the patch provided by Stafford([1]).
> > > 
> > > [1]: http://patches-tcwg.linaro.org/patch/53151/
> > > [2]: https://gitlab.com/bootlin/toolchains-builder/-/jobs
> > 
> > OK, so the real problem is in a backport.  It isn't current mainline
> > binutils configure, which is what I was worried about.
> 
> ...it happens on mainline too but it can be solved by adding -std=c99 to
> CFLAGS.

Do you have an example of it happening on mainline? According to Alan, mainline
should not happen as we should be applying -std=c99 automatically.  If not we
can fix that.

> > BTW, thank you for posting a fix here, even if it isn't strictly
> > necessary for mainline.  Please note that I'm not advocating against
> > your patch.  If target maintainers want to keep their code compatible
> > with C89 that's fine by me.
> > 
> I didn't know about this since no other file failed building on C99. What it
> seems strange to me is that on buildroot binutils seem to be built without
> -std=c99

As mainline binutils is supposed to require c99 now.  I rather not change the
code but fix any issues with configure not setting flags.

I looked at ./configure.ac and I confirm that we have setup c99 flags.  But I
also notice in bfd/configure.ac that we define it again without c99, but that
should not matter as CC should already be defined.  Either way does the below
patch help?

diff --git a/bfd/configure.ac b/bfd/configure.ac
index 07a75ed1626..387c74152d0 100644
--- a/bfd/configure.ac
+++ b/bfd/configure.ac
@@ -34,7 +34,7 @@ dnl Default to a non shared library.  This may be overridden
by the
 dnl configure option --enable-shared.
 AC_DISABLE_SHARED
 
-AC_PROG_CC
+AC_PROG_CC_C99
 AC_GNU_SOURCE
 AC_USE_SYSTEM_EXTENSIONS

--

-Stafford

[OpenRISC] [PATCH v2] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Hi Staffor, Alan, All,

On 6/10/21 9:23 PM, Stafford Horne wrote:
> On Thu, Jun 10, 2021 at 04:48:46PM +0200, Giulio Benetti wrote:
>> Hello Alan, All,
>>
>> On 6/10/21 3:06 PM, Alan Modra wrote:
>>> On Thu, Jun 10, 2021 at 11:07:13AM +0200, Giulio Benetti wrote:
>>>> Hello Alan, All,
>>>>
>>>> On 6/10/21 3:19 AM, Alan Modra wrote:
>>>>> On Wed, Jun 09, 2021 at 11:52:33PM +0200, Giulio Benetti wrote:
>>>>>> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
>>>>>> an old compiler(i.e. gcc 4.9) build fails on:
>>>>>> ```
>>>>>> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
>>>>>> C99 or C11 mode
>>>>>>        for (size_t i = 0; i < insn_count; i++)
>>>>>>        ^
>>>>>> ```
>>>>>
>>>>> Did you find this problem on current mainline binutils?  The configure
>>>>> machinery is supposed to supply the appropriate -std=c99 or -std=gnu99
>>>>> when using older compilers.  That happens for me when I build with
>>>>> gcc-4.9.  I don't think any patch is needed for mainline.
>>>>>
>>>>
>>>> On Buildroot they don't pass -std=c99/g99 and this is the result:
>>>> https://gitlab.com/bootlin/toolchains-builder/-/jobs/1325646298
>>>
>>> That appears to be building binutils 2.35.2
>>>
>>>> This patch seems to follow all the rest code style of binutils
>>>
>>> True, we've only just switched over to requiring C99.
>>
>> Ok so...
>>
>>>> since no
>>>> other part of it fails and this happens only after patch [1] has been
>>>> upstreamed.
>>>>
>>>> Here [2] you can see all the other toolchains built succesfully and only
>>>> Openrisc fails after the patch provided by Stafford([1]).
>>>>
>>>> [1]: http://patches-tcwg.linaro.org/patch/53151/
>>>> [2]: https://gitlab.com/bootlin/toolchains-builder/-/jobs
>>>
>>> OK, so the real problem is in a backport.  It isn't current mainline
>>> binutils configure, which is what I was worried about.
>>
>> ...it happens on mainline too but it can be solved by adding -std=c99 to
>> CFLAGS.
> 
> Do you have an example of it happening on mainline? According to Alan, mainline
> should not happen as we should be applying -std=c99 automatically.  If not we
> can fix that.

I was wrong, it's not on mainline, it's on 2.35.2.

>>> BTW, thank you for posting a fix here, even if it isn't strictly
>>> necessary for mainline.  Please note that I'm not advocating against
>>> your patch.  If target maintainers want to keep their code compatible
>>> with C89 that's fine by me.
>>>
>> I didn't know about this since no other file failed building on C99. What it
>> seems strange to me is that on buildroot binutils seem to be built without
>> -std=c99
> 
> As mainline binutils is supposed to require c99 now.  I rather not change the
> code but fix any issues with configure not setting flags.
> 
> I looked at ./configure.ac and I confirm that we have setup c99 flags.  But I
> also notice in bfd/configure.ac that we define it again without c99, but that
> should not matter as CC should already be defined.  

I've verified that it inherits -std=c99 from upper configure.ac, so no 
need for it. Then please ignore this patch and we will use it backported 
on binutils:
2.32
2.34
2.35.2
2.36.1

in Buildroot. Sorry for the noise :-)

and Best regards
-- 
Giulio Benetti
Benetti Engineering sas

> Either way does the below patch help?
> 
> diff --git a/bfd/configure.ac b/bfd/configure.ac
> index 07a75ed1626..387c74152d0 100644
> --- a/bfd/configure.ac
> +++ b/bfd/configure.ac
> @@ -34,7 +34,7 @@ dnl Default to a non shared library.  This may be overridden
> by the
>   dnl configure option --enable-shared.
>   AC_DISABLE_SHARED
>   
> -AC_PROG_CC
> +AC_PROG_CC_C99
>   AC_GNU_SOURCE
>   AC_USE_SYSTEM_EXTENSIONS
> 
> --
> 
> -Stafford
> 

[OpenRISC] [PATCH] bfd/elf32-or1k: fix building with gcc version < 5

[Giulio Benetti]

Hi Stafford,

On 6/9/21 11:44 PM, Stafford Horne wrote:
> On Wed, Jun 09, 2021 at 05:30:59PM +0200, Giulio Benetti wrote:
>> Gcc version >= 5 has standard C mode not set to -std=gnu11, so if we use
>> an old compiler(i.e. gcc 4.9) build fails on:
>> ```
>> elf32-or1k.c:2251:3: error: 'for' loop initial declarations are only allowed in
>> C99 or C11 mode
>>      for (size_t i = 0; i < insn_count; i++)
>>      ^
>> ```
>>
>> So let's declare `size_t i` at the top of the function instead of inside
>> for loop.
> 
> This looks ok to me.  Can you also include the changelog entry needed for
> binutils patches?
> 
> Something like:
> 
> bfd/ChangeLog:
> 
> 	* elf32-or1k.c (or1k_write_plt_entry): Move i declaration to top of
> 	function.

Sure, this it the first time I send a patch for binutils. I've just sent 
the v2.

Best regards
-- 
Giulio Benetti
Benetti Engineering sas

>> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
>> ---
>>   bfd/elf32-or1k.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/bfd/elf32-or1k.c b/bfd/elf32-or1k.c
>> index 4ae7f324d33..32063ab0289 100644
>> --- a/bfd/elf32-or1k.c
>> +++ b/bfd/elf32-or1k.c
>> @@ -2244,9 +2244,10 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
>>   {
>>     unsigned nodelay = elf_elfheader (output_bfd)->e_flags & EF_OR1K_NODELAY;
>>     unsigned output_insns[PLT_MAX_INSN_COUNT];
>> +  size_t i;
>>   
>>     /* Copy instructions into the output buffer.  */
>> -  for (size_t i = 0; i < insn_count; i++)
>> +  for (i = 0; i < insn_count; i++)
>>       output_insns[i] = insns[i];
>>   
>>     /* Honor the no-delay-slot setting.  */
>> @@ -2277,7 +2278,7 @@ or1k_write_plt_entry (bfd *output_bfd, bfd_byte *contents, unsigned insnj,
>>       }
>>   
>>     /* Write out the output buffer.  */
>> -  for (size_t i = 0; i < (insn_count+1); i++)
>> +  for (i = 0; i < (insn_count+1); i++)
>>       bfd_put_32 (output_bfd, output_insns[i], contents + (i*4));
>>   }
>>   
>> -- 
>> 2.25.1
>>