foss-for-synopsys-dwc-arc-processors / buildroot

The development tree for Buildroot support for the Synopsys DesignWare ARC processor family
https://github.com/foss-for-synopsys-dwc-arc-processors/toolchain/blob/arc-dev/README.md
Other
7 stars 2 forks source link

Add ARC700 target for nSIM #30

Closed geomatsi closed 2 years ago

geomatsi commented 2 years ago

Add ARC700 target for nSIM including support for both uClibc and glibc. Note that ARC700 target is not supported by glibc out of the box. Add out-of-tree patch to enable that support for testing and development purposes. By default build configuration snps_arc700_nsim_defconfig enables uClibc. Use menuconfig to switch to glibc.

geomatsi commented 2 years ago
  • Move glibc patch into package/glibc/xxx-whatever-snapshot-is-used.

I think it makes sense to keep patch in the board specific directory. No need to apply that patch for any glibc build since it is required only for rare ARC700 glibc build. That is going to minimize possible impact of glibc build failure due to any arc changes in the future.

  • Create a git-like .patch with the commit title, commit message & your SoB. In the commit message I'd put the following info:

    • Origin of the patch: OpenWrt will do, especially if there's a reference to an earlier discussion - so that the future readers may figure out all the details of what is that and why it's done that way.
    • A note on a fix you had to add for the dynamic linker symlink... and BTW it is worth adding that fix to the OpenWrt (where AFAIK we used to see a problem in run-time, so @EvgeniiDidin might be interested in that part).
    • Why it won't be a part of the upstream glibc anytime soon (but pls be careful here to not put us in a silly situation). I'd say something like "Even though ARC700 port of the glibc is very-very similar to ARCv2 port, still as formally yet another ABI it requires proper maintenance in the upstream like running very extensive test-suite, be in charge of occasional failure fixes etc. And since ARCompact ISA is reaching its EOL it might not be very practical to invest so much work in something which may have use, but not that often - uClibc is much more used on ARC700. Still given the low-hanging fruit, let's make some people a bit happier and let them run those libs & apps which are not usable with uClibc".

Done. I split glibc patch into the following two patches:

Also see the commit messages in those two patches.

  • Get rid of the .props file and instead add all of those to the readme.txt in form of nsimdrv -prop=xxx -prop=yyy ....

The point is nobody cares about readme contents, while an extra file which is not generally super useful for the upstream project might raise some questions. Also while we're at it and refer to the nSIM, let's add some more visibility to that and point to where those interested in it might really obtain the tool: https://www.synopsys.com/cgi-bin/dwarcnsim/req1.cgi.

Done. Got rid of arc700.prop and added links to free nSIM version for Linux.

Regards, Sergey

abrodkin commented 2 years ago

I'm still not convinced with move of ARC700 patch for glibc outside of package/glibc/ folder. My concerns are:

  1. We're talking about change applicable to entire arch, not a separate platform. Thus it's not reasonable to rely on a defconfig which is by definition per board, not per platform. I.e. those who may want to use BR for their own platform may very well start from scratch and miss that patch from some "nSIM" board.
  2. It's just you being lucky to have glibc available for ARCompact. See https://github.com/buildroot/buildroot/blob/master/toolchain/toolchain-buildroot/Config.in#L52:
    config BR2_TOOLCHAIN_BUILDROOT_GLIBC
    depends on (BR2_arc && BR2_ARC_ATOMIC_EXT)

    it was introduced back in the day when we used glibc from the fork on our GitHub where ARCompact/ARC700 was supported (that patch was integrated in the source tree) but once we switched to the upstream release of glibc that line should have been changed into something like:

    config BR2_TOOLCHAIN_BUILDROOT_GLIBC
    depends on (BR2_arc && !(BR2_arc750d || BR2_arc770d))

Otherwise you still may select glibc for ARC700 but it will fail to build - that's wrong. And since we don't want to add dependency in toolchain setting on a particular platform (nSIM), we have to make it a generic solution - apply ARC700 patch all the time.

abrodkin commented 2 years ago

Also I'm not sure if we really need to break the patch into two smaller ones as it will require a bit more maintenance. Instead we need to send a missing part to OpenWrt and call it a day.

geomatsi commented 2 years ago

Ok. Done

abrodkin commented 2 years ago

Sorry, I'm a terrible reviewer ;) Another "brilliant" ideas of me:

  1. Maybe rename "package/glibc: add support for ARC700" to something like "... re-add support ..." since as I mentioned earlier ARC700 support in glibc was in Buildroot until we switched to the upstream releases and then it silently was gone... so that nobody ever noticed until you found it by surprise.
  2. Add that note to the Buildroot's commit message above note so that Thomas & friends are not too much surprised :)
geomatsi commented 2 years ago

Closing for now. We need a more generic cleanup that will introduce 3 generations or ARC chips.

abrodkin commented 2 years ago

Let's finally get it done so glibc with ARC700 is usable and builds won't fail like https://gitlab.com/buildroot.org/buildroot/-/jobs/3101769991.