search

sifive / freedom-tools

Tools for SiFive's Freedom Platform
217 stars 52 forks source link

Bump binutils #18

Closed kito-cheng closed 5 years ago

kito-cheng commented 5 years ago

This bug was discovered last few days, and Jim has fixed it on upstream soon, more detail from Jim's commit log:

RISC-V: Fix a gp relaxation reloc overflow error.

This was broken when I changed how we compute the value for the gp register.
It used to be computed inside the sdata section.  Now it is computed at the
end which makes it an abs section symbol.  There is code that tries to use
the alignment of the section that the gp value is in, but this does not work
if it is in the abs section, as the abs section has alignment of 1 byte.
There are people using alternative linker scripts that still define it in the
sdata section, so the code is still useful.  Thus adding a check to disable
this when gp is in the abs section.

    bfd/
    * elfnn-riscv.c (_bfd_riscv_relax_lui): Add check to exclude abs
    section when setting max_alignment.  Update comment.
    (_bfd_riscv_relax_pc): Likewise.
cgsfv commented 5 years ago

Should I start a new build of Freedom Tools Toolchain - RC3?

jim-wilson commented 5 years ago

I just got another linker bug report from Kito, so there might be another patch coming.

cgsfv commented 5 years ago

Fine - I will just wait a bit more...

jim-wilson commented 5 years ago

The new linker bug is shared library specific, which means it is linux specific, so it is outside the scope of freedom-tools. I have a patch, I expect to check it in upstream tonight or tomorrow.

kito-cheng commented 5 years ago

Another 2 patches are back port from upstream, this PR should ready to merge, wait @jim-wilson ack.

RISC-V: Fix linker problems with tls copy relocs.

The linker doesn't allocate memory space for sections that are only SEC_ALLOC
and SEC_THREAD_LOCAL.  See the IS_TBSS test in ld/ldlang.c.  So we need to
pretend that .tdata.dyn sections have contents to get the right result.  It
will be marked this way anyways if there is a .tdata section to merge with.
RISC-V: Force linker error exit after unresolvable reloc.

This was noticed while trying to test the compiler -msave-restore support.
Putting non-pic code in a shared library gives a linker error, but doesn't
stop the build.
cgsfv commented 5 years ago

I will ask once again: Should I start a new build of Freedom Tools Toolchain - RC3?

jim-wilson commented 5 years ago

The binutils changes haven't been merged in yet. It makes more sense to start a build after the latest linker patches are merged in.

The two binutils changes look OK to me. Though the tls copy relocs problem is Linux specific, and I'm not sure why Kito wants to merge that one is, as freedom-tools doesn't support linux. It should be harmless though, as it won't affect embedded elf builds.

cgsfv commented 5 years ago

No problem, just give me a ping when I should start the build.

jim-wilson commented 5 years ago

Who is merging in the patches?

kito-cheng commented 5 years ago

Hi @cgsfv, I think Jim's meaning is this PR is OK to him, you can build RC3 after merge this PR.

but here is another bug report [1] from FSF's gcc bugzilla, it generate wrong code for RV64, so I think it would be better to including that in this release.

However I think you can merge this PR first, and I will sent another PR for fixing GCC.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91635

jim-wilson commented 5 years ago

Adding random bug fixes before a release isn't a good idea. The closer to a release we are, the less testing we can do, and the more likely a patch breaks more than it fixes. If a customer has reported a bug, or someone internally has reported a bug, then that should be fixed. If someone upstream reports an obscure bug, then there is no particular reason to include that fix. Also, if you wait a week, someone is likely to find yet another bug. We need to draw a line somewhere and I think we are already past that line. We shouldn't add any more bug fix patches unless we have a verifiable need for them.

I think the ones in this pull request are OK, because first few were internally reported, and the rest are linux specific and hence will have no effect on our embedded elf release. But we really need to stop adding patches for no reason.

kito-cheng commented 5 years ago

I agree your point, and seems like we already delay, and that bug need more time to testing and reviewing.

So yes, I am OK to postpone the GCC fix to next release.

kito-cheng commented 5 years ago

Hi @cgsfv , so let's merge and build for RC3 :)

cgsfv commented 5 years ago

Merged done and build has started.

kito-cheng commented 5 years ago

Thanks :)