[v2,00/12] RFC: ARC port

Dear Mischa Jonker,

On Wed, 24 Apr 2013 12:13:11 +0200, Mischa Jonker wrote:

> Thanks for the prompt feedback that I have received on the patches.
> Here's a new revision of the patchset to introduce ARC support in
> BuildRoot; I think I have addressed all the comments received from
> Thomas and Arnout.

Thanks for the quick turnaround! This time, I actually applied and
tested your patches in the autobuilders environment, and I had some
issues.

The first issue is that your version of binutils require flex and bison
installed on the host, so we have to add these as dependencies of
binutils. The binutils.patch attached does that, but it does that
unconditionally, regardless of the binutils version being chosen. Since
the recent binutils versions do not require building flex and bison,
I'd prefer to make this dependency conditional on which version of
binutils is being built.

The second issue is that gcc 4.4.x also require flex (and maybe bison,
but I haven't checked). So I've added host-flex and host-bison as gcc
dependencies. Unfortunately, gcc doesn't use the package
infrastructure, so it doesn't add host/usr/bin to the PATH, so I had to
change the gcc makefile to use $(HOST_MAKE_ENV) and $(TARGET_MAKE_ENV)
where appropriate. This is the second 'gcc.patch' attached does. Here
as well, it makes the host-flex and host-bison dependency
unconditional, even though the latest versions of gcc do not require
this. A proper patch would have to make this dependency conditional on
the version of gcc being used.

(Note: both patches are quick and dirty, they don't have any
description or signed-off-by. Feel free to re-use them and apply your
signed-off-by on them, I don't care about authorship on those patches.)

Note that those flex and bison issues are visible in the autobuilder
environment because the build runs in a minimal chroot that has only
the strict, minimal dependencies required by Buildroot. That probably
explains why you haven't seen those problems until now.

Then, once this was fixed, the build went on, but failed at the gcc
intermediate step, with some pthread related issue:

In file included from /home/test/outputs/arc/toolchain/uClibc_dev//usr/include/stdio.h:71,
                 from /home/test/outputs/arc/toolchain/gcc-4.4.7-arc/libgcc/../gcc/tsystem.h:87,
                 from /home/test/outputs/arc/toolchain/gcc-4.4.7-arc/libgcc/../gcc/libgcc2.c:29:
/home/test/outputs/arc/toolchain/uClibc_dev//usr/include/bits/uClibc_stdio.h:207: error: expected specifier-qualifier-list before 'pthread_mutex_t'

make[3]: *** [_negdi2.o] Error 1
make[3]: *** Waiting for unfinished jobs....

The defconfig I was trying to build is very simple:

$ cat defconfig 
BR2_arcle=y

Do you have an idea for this pthread_mutex_t issue? Maybe ARC doesn't
support NPTL or something?

Also, do you have a public repo to push your changes to? Since your
patch series has become quite long, it would make it easier for us to
test it as a whole.

Thanks!

Thomas

Hi Thomas,

> (host-flex/host-bison)
Thanks for the patches! I have integrated them and added version checks. I'm not 100% certain from which version onwards the dependency has been removed, but my colleague says that he didn't encounter these dependencies with GCC 4.5, so I made it depend on 4.2,4.3,4.4.

> Do you have an idea for this pthread_mutex_t issue? Maybe ARC doesn't support NPTL or something?
Correct, we don't have NPTL support yet. It was the initial planning to have this finished before we went out with the BuildRoot patches, but priorities shifted, and I hadn't disabled it yet. Added a patch for that.

> Also, do you have a public repo to push your changes to? Since your patch series has become quite long, it would make it easier for us to test it as a whole.
Our github site has a BuildRoot repository:
https://github.com/foss-for-synopsys-dwc-arc-processors/buildroot

I'm regularly rebasing & updating patches, and pushing them to this repo. Right now, there are two branches/tags that are relevant:
* master_arc_20130425
* master_3.9_arc_20130425

The only difference between the two is that the first relies on 3.9-rc8, and the second on 3.9. So the second branch doesn't build right now, but should build after this weekend.

BTW: would you like to repost the new set of patches on the mailing list, or would you rather have a look at it on github?
BTW2: I noticed that GNU config went from GPLv2 to GPLv3, I'm not sure what the impact would be for BuildRoot, as it is really part of BuildRoot (not downloaded).

Mischa

Dear Mischa Jonker,

On Thu, 25 Apr 2013 11:01:15 +0000, Mischa Jonker wrote:

> > (host-flex/host-bison)
> Thanks for the patches! I have integrated them and added version
> checks. I'm not 100% certain from which version onwards the
> dependency has been removed, but my colleague says that he didn't
> encounter these dependencies with GCC 4.5, so I made it depend on
> 4.2,4.3,4.4.

Ok.

> > Do you have an idea for this pthread_mutex_t issue? Maybe ARC
> > doesn't support NPTL or something?
> Correct, we don't have NPTL support yet. It was the initial planning
> to have this finished before we went out with the BuildRoot patches,
> but priorities shifted, and I hadn't disabled it yet. Added a patch
> for that.

Good.

> > Also, do you have a public repo to push your changes to? Since your
> > patch series has become quite long, it would make it easier for us
> > to test it as a whole.
> Our github site has a BuildRoot repository:
> https://github.com/foss-for-synopsys-dwc-arc-processors/buildroot
> 
> I'm regularly rebasing & updating patches, and pushing them to this
> repo. Right now, there are two branches/tags that are relevant:
> * master_arc_20130425
> * master_3.9_arc_20130425
> 
> The only difference between the two is that the first relies on
> 3.9-rc8, and the second on 3.9. So the second branch doesn't build
> right now, but should build after this weekend.

3.9 has been released now, so that should solve your problem. Could you
rebase your patch series on top of master, Peter has added 3.9 as a
supported kernel.

I tested your branch, and it built fine for me on the autobuilders, so
in terms of basic buildability, we're good.

> BTW: would you like to repost the new set of patches on the mailing
> list, or would you rather have a look at it on github? BTW2: I
> noticed that GNU config went from GPLv2 to GPLv3, I'm not sure what
> the impact would be for BuildRoot, as it is really part of BuildRoot
> (not downloaded).

Yes, they should be reposted, but I have a number of comments on them:

 * 'toolchain/gcc: Add host-{flex,bison} dependencies for GCC
   4.2,4.3,4.4' should be earlier in the series, i.e before 'arc: add
   gcc for ARC'.

 * 'package/binutils: add host-{flex,bison} dependencies for 2.19-arc'
   should probably be squashed into 'arc: Add support for ARC-specific
   binutils'.

 * I am not entirely happy with 'toolchain/gcc: Introduce
   BR2_ARCH_HAS_NO_GCC_x_y' using a reverted logic. I haven't thought
   too much about it, but wouldn't it be possible to instead do
   something like BR2_ARCH_NEEDS_GCC_4_6_PLUS, and then do something
   like depends !BR2_ARCH_NEEDS_GCC_4_6_PLUS on all gcc versions <
   4.6 ? Hum, the problem is that it works with architecture that are
   supported starting from a given upstream gcc version, but not
   architecture that are supported only in 4.2-avr, or in 4.4-arc, etc.

   Maybe others will have a suggestion on this point?

Other than that, it looks good to me.

Thomas

On 29/04/13 22:45, Thomas Petazzoni wrote:
>   * I am not entirely happy with 'toolchain/gcc: Introduce
>     BR2_ARCH_HAS_NO_GCC_x_y' using a reverted logic. I haven't thought
>     too much about it, but wouldn't it be possible to instead do
>     something like BR2_ARCH_NEEDS_GCC_4_6_PLUS, and then do something
>     like depends !BR2_ARCH_NEEDS_GCC_4_6_PLUS on all gcc versions <
>     4.6 ? Hum, the problem is that it works with architecture that are
>     supported starting from a given upstream gcc version, but not
>     architecture that are supported only in 4.2-avr, or in 4.4-arc, etc.
>
>     Maybe others will have a suggestion on this point?

  Not entirely clean either, but what about the following:

[toolchain/gcc/Config.in]
config BR2_ARCH_HAS_ALL_GCC
	bool

config BR2_ARCH_HAS_GCC_4_4
	bool
	default y if BR2_ARCH_HAS_ALL_GCC

[arch/Config.in]
config BR2_mips
	select BR2_ARCH_HAS_ALL_GCC
...

[arch/Config.in.arm]
config BR2_arm1176jzf_s
	select BR2_ARCH_HAS_ALL_GCC
...
config BR2_cortex_a8
	select BR2_ARCH_HAS_GCC_4_4
...



  Regards,
  Arnout

Dear Arnout Vandecappelle,

On Tue, 30 Apr 2013 18:06:27 +0200, Arnout Vandecappelle wrote:
> On 29/04/13 22:45, Thomas Petazzoni wrote:
> >   * I am not entirely happy with 'toolchain/gcc: Introduce
> >     BR2_ARCH_HAS_NO_GCC_x_y' using a reverted logic. I haven't thought
> >     too much about it, but wouldn't it be possible to instead do
> >     something like BR2_ARCH_NEEDS_GCC_4_6_PLUS, and then do something
> >     like depends !BR2_ARCH_NEEDS_GCC_4_6_PLUS on all gcc versions <
> >     4.6 ? Hum, the problem is that it works with architecture that are
> >     supported starting from a given upstream gcc version, but not
> >     architecture that are supported only in 4.2-avr, or in 4.4-arc, etc.
> >
> >     Maybe others will have a suggestion on this point?
> 
>   Not entirely clean either, but what about the following:
> 
> [toolchain/gcc/Config.in]
> config BR2_ARCH_HAS_ALL_GCC
> 	bool
> 
> config BR2_ARCH_HAS_GCC_4_4
> 	bool
> 	default y if BR2_ARCH_HAS_ALL_GCC
> 
> [arch/Config.in]
> config BR2_mips
> 	select BR2_ARCH_HAS_ALL_GCC
> ...
> 
> [arch/Config.in.arm]
> config BR2_arm1176jzf_s
> 	select BR2_ARCH_HAS_ALL_GCC
> ...
> config BR2_cortex_a8
> 	select BR2_ARCH_HAS_GCC_4_4
> ...

Could be good. That said, this problem is quite different from doing
the ARC port. So maybe we could instead tell Mischa to revert back to
its original solution (adding yet another set of !BR2_arc everywhere),
and we can work on a solution to improve this as a follow-up patch. Of
course, if Mischa is willing to work on this, that'd be great, but I
don't think it's really mandatory to make this cleanup a strict
requirement to get the ARC code merged. The existing set of 'depends
on' for gcc versions is already ugly, it's not specifically the ARC
stuff that makes it ugly.

Thomas

> >     Maybe others will have a suggestion on this point?
> 
>   Not entirely clean either, but what about the following:
> 
> [toolchain/gcc/Config.in]
> config BR2_ARCH_HAS_ALL_GCC
> 	bool
> 
> config BR2_ARCH_HAS_GCC_4_4
> 	bool
> 	default y if BR2_ARCH_HAS_ALL_GCC
> 
> [arch/Config.in]
> config BR2_mips
> 	select BR2_ARCH_HAS_ALL_GCC
> ...
> 
> [arch/Config.in.arm]
> config BR2_arm1176jzf_s
> 	select BR2_ARCH_HAS_ALL_GCC
> ...
> config BR2_cortex_a8
> 	select BR2_ARCH_HAS_GCC_4_4
> ...

The problem with this is that you end up in Config.in.arm with a lot of "select BR2_ARCH_HAS_ALL_GCC" lines. It is more logical, but it adds a lot of clutter IMHO. 

Unfortunately, we cannot make "BR2_ARCH_HAS_ALL_GCC" default y, because we don't have the 'deselect' keyword, so we need to add it everywhere. For ARM and x86, the architectures that aren't supported by all GCC versions are the exception, so instead of adding additional lines for just the exceptions, we end up with extra code everywhere.

I also tried to let default values depend on earlier versions:
config BR2_ARCH_HAS_GCC_4_5
	bool
	default y if BR2_ARCH_HAS_GCC_4_4

But this doesn't work for several architectures, such as BR2_sparc_sparcsfleon, that do have GCC 4.4, but no 4.5 for instance.

The reverse logic helps in the sense that the BR2_HAS_NO_GCC_x_y lines will go away over time when old GCC versions get deprecated.

I will send both variants later today.

Mischa

Dear Mischa Jonker,

On Thu, 2 May 2013 14:03:53 +0000, Mischa Jonker wrote:

[...]

> But this doesn't work for several architectures, such as
> BR2_sparc_sparcsfleon, that do have GCC 4.4, but no 4.5 for instance.
> 
> The reverse logic helps in the sense that the BR2_HAS_NO_GCC_x_y
> lines will go away over time when old GCC versions get deprecated.
> 
> I will send both variants later today.

As I suggested in an earlier e-mail, I think you should just not care
about this problem for now, and add the multiple 'depends on !BR2_arc'
as you were doing in your first version of the patch set. Your patch
set already has a good number of patches, so let's merge it, and
improve on top of that.

Thanks!

Thomas

Hi Thomas,

>> I will send both variants later today.

> As I suggested in an earlier e-mail, I think you should just not care about 
> this problem for now, and add the multiple 'depends on !BR2_arc'
> as you were doing in your first version of the patch set. Your patch set 
> already has a good number of patches, so let's merge it, and improve on 
> top of that.

The v3 patch set does exactly that if I'm not mistaken. This is supposed to be
the improvement part already:-)

Mischa

Dear Mischa Jonker,

On Thu, 2 May 2013 15:18:50 +0000, Mischa Jonker wrote:

> > As I suggested in an earlier e-mail, I think you should just not care about 
> > this problem for now, and add the multiple 'depends on !BR2_arc'
> > as you were doing in your first version of the patch set. Your patch set 
> > already has a good number of patches, so let's merge it, and improve on 
> > top of that.
> 
> The v3 patch set does exactly that if I'm not mistaken. This is supposed to be
> the improvement part already:-)

Yeah, seen that. I replied before I saw your v3, and the gcc patches
that come after that. I'll focus on the ARC stuff for now.

Thanks!

Thomas

On 02/05/13 16:03, Mischa Jonker wrote:
>>>      Maybe others will have a suggestion on this point?
>>
>>    Not entirely clean either, but what about the following:
>>
>> [toolchain/gcc/Config.in]
>> config BR2_ARCH_HAS_ALL_GCC
>> 	bool
>>
>> config BR2_ARCH_HAS_GCC_4_4
>> 	bool
>> 	default y if BR2_ARCH_HAS_ALL_GCC
>>
>> [arch/Config.in]
>> config BR2_mips
>> 	select BR2_ARCH_HAS_ALL_GCC
>> ...
>>
>> [arch/Config.in.arm]
>> config BR2_arm1176jzf_s
>> 	select BR2_ARCH_HAS_ALL_GCC
>> ...
>> config BR2_cortex_a8
>> 	select BR2_ARCH_HAS_GCC_4_4
>> ...
>
> The problem with this is that you end up in Config.in.arm with a lot of "select BR2_ARCH_HAS_ALL_GCC" lines. It is more logical, but it adds a lot of clutter IMHO.

  Okay, yet another idea. I don't know if this will actually work.

[toolchain/gcc/Config.in]
config BR2_ARCH_HAS_ALL_GCC
	bool
	default y if !(BR2_ARCH_HAS_GCC_4_4 || BR2_ARCH_HAS_GCC_4_5 ...)

config BR2_ARCH_HAS_GCC_4_4
	bool

config BR2_GCC_VERSION_4_4_X
	depends on BR2_ARCH_HAS_ALL_GCC || BR2_ARCH_HAS_GCC_4_4


> Unfortunately, we cannot make "BR2_ARCH_HAS_ALL_GCC" default y, because we don't have the 'deselect' keyword, so we need to add it everywhere. For ARM and x86, the architectures that aren't supported by all GCC versions are the exception, so instead of adding additional lines for just the exceptions, we end up with extra code everywhere.
>
> I also tried to let default values depend on earlier versions:
> config BR2_ARCH_HAS_GCC_4_5
> 	bool
> 	default y if BR2_ARCH_HAS_GCC_4_4
>
> But this doesn't work for several architectures, such as BR2_sparc_sparcsfleon, that do have GCC 4.4, but no 4.5 for instance.
>
> The reverse logic helps in the sense that the BR2_HAS_NO_GCC_x_y lines will go away over time when old GCC versions get deprecated.

  Yes, looking at the alternatives, the negative logic isn't that bad...

  However, the advantage of the positive logic is that it is slightly 
more future-safe. When adding a new gcc version, you have to remember to 
also add all the negative cases. Of course, you also have to remember to 
add a new positive case, but it is less bad if you forget that.

>
> I will send both variants later today.

  As Thomas said, don't feel obliged to continue working on this refactoring.


  Regards,
  Arnout

> Mischa
>
>