The -mrelax support is broken for sh-elf since PIC code support was added. Most of the work to fix this will likely be in bfd, ld and gas, although some gcc changes may also be necessary. IIRC I've fixed the switch tables once, but ordinary branches are still broken. gcc R117374 with binutils/newlib D2006.10.02.18.00.00 shows 681 failures for check-gcc check-target-libstdc++-v3 testing the variations sh-hms-sim sh-hms-sim/-m4/-ml with a small patch to enable -mrelax when optimizing, the tests are not finished after 17.5 hours, but 2098 failures have been detected so far.
Created attachment 1346 [details] patch to enable relaxation when optimizing This patch to gcc/gcc/config/sh/sh.h enables -mrelax by default when optimizing, thus exposing the brokenness of this feature for an ordinary build / test cycle.
(In reply to comment #0) > with a small patch to enable -mrelax when optimizing, the tests are not finished > after 17.5 hours, but 2098 failures have been detected so far. Still not finished after 64.9 hours, 4074 failures so far.
One thing that I just noticed while browsing through sh_elf_relax_delete_bytes is that we don't process R_SH_ALIGN properly; i.e. when we stop at a larger alignment, we should should check if we should now remove bytes of the full size of the alignment.
Created attachment 8080 [details] GCC patch to enable -mrelax by default This is another patch to turn the -mrelax option in GCC by default. Since PR 10378 and PR 10373 have been fixed, I've tried building GCC with -mrelaxed enabled by default. The build would stop in libgcc/config/sh/lib1funcs.S, where GAS would complain that a mova insn is referring to an unaligned offset. After applying the following: Index: libgcc/config/sh/lib1funcs.S =================================================================== --- libgcc/config/sh/lib1funcs.S (revision 220017) +++ libgcc/config/sh/lib1funcs.S (working copy) @@ -116,7 +116,7 @@ HIDDEN_FUNC(GLOBAL(ashiftrt_r4_31)) HIDDEN_FUNC(GLOBAL(ashiftrt_r4_32)) - .align 1 + .align 4 GLOBAL(ashiftrt_r4_32): GLOBAL(ashiftrt_r4_31): rotcl r4 @@ -765,6 +765,7 @@ bt GLOBAL(movmemSI52) ! done all the large groups, do the remainder ! jump to movmem+ + .balign 4 mova GLOBAL(movmemSI4)+4,r0 add r6,r0 jmp @r0 GCC (sh-elf C,C++, newlib 2.2.0) can be built successfully. When compiling a simple C++ program with the resulting toolchain: #include <iostream> int main (void) { std::cout << "hello world" << std::endl; return 0; } There are a lot of warnings from LD like: /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/bin/ld: /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/lib/ml/m4/libstdc++.a(locale_init.o): 0x18: warning: symbol in unexpected section /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/bin/ld: /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/lib/ml/m4/libstdc++.a(locale_init.o): 0x6c: warning: symbol in unexpected section /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/bin/ld: /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/lib/ml/m4/libstdc++.a(locale_init.o): 0x70: warning: symbol in unexpected section /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/bin/ld: /usr/local/lib/gcc/sh-elf/5.0.0/../../../../sh-elf/lib/ml/m4/libstdc++.a(locale_init.o): 0x3c: warning: symbol in unexpected section But linking somehow succeeds. However, the resulting program does not work.
PR 32917 has a patch (fix) for the `warning: symbol in unexpected section` spam
Created attachment 16079 [details] Patch to fix load/store swap crash bug I am now able to execute my bare metal binarys (non-PIC)
(In reply to Joern Rennecke from comment #3) > One thing that I just noticed while browsing through > sh_elf_relax_delete_bytes > is that we don't process R_SH_ALIGN properly; i.e. when we stop at a larger > alignment, we should should check if we should now remove bytes of the full > size of the alignment. Could you please elaborate on that? I was not able to find a problem in the current handling of aligns, maybe an example would help me.
The master branch has been updated by Alan Modra <amodra@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6cd9586f7f502d9034751df1b88b1ae91a34a8b6 commit 6cd9586f7f502d9034751df1b88b1ae91a34a8b6 Author: QBos07 <qubos@outlook.de> Date: Fri May 9 13:49:44 2025 +0000 PR 3298 Fix SuperH relaxation overriding wrong intruction when doing load store switching it wrongly adjusts the address of the R_SH_USES reloc and not the actual offset from that instruction. This is an issue if the pc-relative function call relaxation gets done in a later pass wich will result in overriding the wrong instruction.
(In reply to Quentin Boswank from comment #7) > (In reply to Joern Rennecke from comment #3) > > One thing that I just noticed while browsing through > > sh_elf_relax_delete_bytes > > is that we don't process R_SH_ALIGN properly; i.e. when we stop at a larger > > alignment, we should should check if we should now remove bytes of the full > > size of the alignment. > > Could you please elaborate on that? I was not able to find a problem in the > current handling of aligns, maybe an example would help me. Hmm, it seems I have missed the code at the end of the function, after the comment: /* See if we can move the ALIGN reloc forward. We have adjusted r_offset for it already. */ . In my defence, it's about 450 lines down from the code that initially handled the alignment, and the comment for the latter talks about stopping the deletion, without a hint that it's going to be taken up later. /* The deletion must stop at the next ALIGN reloc for an alignment power larger than the number of bytes we are deleting. */ Maybe that comment could be expanded upon, something like that? /* The deletion must stop at the next ALIGN reloc for an alignment power larger than the number of bytes we are deleting; we'll check later if such an alignment can be trimmed too. */ Also, looking at the code, it occurs to me that it is a bit brittle. Although this can't happen at the moment, if some new relaxation called sh_elf_relax_delete_bytes with a count value that is not a power of two (e.g. six), we could end up with R_SH_ALIGN relocs that are not aligning properly. We could add an assert, like: if (ELF32_R_TYPE (irel->r_info) == (int) R_SH_ALIGN && irel->r_offset > addr) { if (count < (1 << irel->r_addend)) { irelalign = irel; toaddr = irel->r_offset; break; } BFD_ASSERT ((count & ((1 << irel->r_addend) - 1)) == 0); } or simpler, just handle the hypothetical situation - although we'd lack testing, and the comment should probably be adjusted too: if (ELF32_R_TYPE (irel->r_info) == (int) R_SH_ALIGN && irel->r_offset > addr && (count & ((1 << irel->r_addend) - 1)) != 0) { irelalign = irel; toaddr = irel->r_offset; break; } N.B. I'm not going to test any of this, I have no current connection with the SH port.
There seems to be a bug in gas where switch table entris will not emit R_SH_SWITCH8 relocs when using -ffunction-sections in gcc. The R_SH_CODE/DATA pair around them however will still get generated. Minimal example: .section .text.hi,"ax",@progbits .align 2 .L1: .byte .L2-.L1 .align 1 .L2:
(In reply to Quentin Boswank from comment #6) > Created attachment 16079 [details] > Patch to fix load/store swap crash bug > > I am now able to execute my bare metal binarys (non-PIC) I see we have similar code in coff-sh.c sh_swap_insns. Was the bug copied from there, or did it arise because of copying without accounting for coff/elf reloc differences?
Created attachment 16103 [details] Patch for gas bug not creating switch table relocs in alternative code sections quick 2 liner fix somewhat expected something like this
(In reply to Joern Rennecke from comment #11) > (In reply to Quentin Boswank from comment #6) > > Created attachment 16079 [details] > > Patch to fix load/store swap crash bug > > > > I am now able to execute my bare metal binarys (non-PIC) > > I see we have similar code in coff-sh.c sh_swap_insns. > Was the bug copied from there, or did it arise because of copying without > accounting for coff/elf reloc differences? Could very well be copied over bug but I do not know enought about coff to give you a resonable answere here
Created attachment 16105 [details] patch for gas bug zeroing 16 or 32 bit width jumptables when relaxing Off course there was another one
(In reply to Quentin Boswank from comment #14) > Created attachment 16105 [details] > patch for gas bug zeroing 16 or 32 bit width jumptables when relaxing > > Off course there was another one + max = (1 << (fixP->fx_size * 8)) - 1; In C, 1 is of type int. When fixP->fx_size is 4, this causes undefined behaviour if int is 32 bit (on the host). Also, I fear your replacing of apply_full_field_fix with md_number_to_chars might break something else - e.g. a constant with a label or symbol value used for a pc-relative load. Have you considered changing apply_full_field_fix instead to check for SWITCH_TABLE (fixP) ? i.e.: if ((fixP->fx_addsy != NULL || fixP->fx_pcrel) && !SWITCH_TABLE (fixP))
(In reply to Joern Rennecke from comment #11) > I see we have similar code in coff-sh.c sh_swap_insns. Yes. That is obviously wrong too. Likely adjusting r_addend isn't the right thing to do for COFF, we'd probably need to adjust section contents instead. If we had an assembly language testcase with R_SH_USES involved in an insn swap for sh-elf then even I could look into the coff-sh case.
Created attachment 16107 [details] patch for gas bug zeroing 16 or 32 bit width jumptables when relaxing V2
I've dugged depper into gas and oh boy is it ever brocken! I have tested with all the patches appied because the patches themselfs are fine but they allow for other problems to get visible. Here are some of the things I found: - no R_SH_REL8/16 (R_SH_DIR will get emitted instead) - getting a +3 addend but the directive was +1 on .byte and .word - addends missing for R_SH_REL/DIR32 - wrong R_SH_DATA/CODE (even saw both of them at the same address) - missing and unneeded R_SH_LABELs - no destinction if a label is an actual jump target or just for .uses as an exmaple (blocks store/load relaxtion swapping in many cases) - getting diffrent relocs depending on wich section I am in (usually should be the same) - other wierd reloc aliasing and missing ones like switch table logic I am suprised that gas works so well because some of these afect it even without relaxation. I will not fix all of them because I just dont have enought time for this alongside school.
(In reply to Quentin Boswank from comment #18) > > I will not fix all of them because I just dont have > enought time for this alongside school. That's alright. It's been broken for such a long time, I don't think anybody expects this to get fixed overnight. Thanks for looking into it!
(In reply to Quentin Boswank from comment #17) > Created attachment 16107 [details] > patch for gas bug zeroing 16 or 32 bit width jumptables when relaxing V2 max = (1ul << (fixP->fx_size * 8)) - 1; There are many ways to avoid shifting by a shiftcount as large or larger than type bing shifted. This is not one of them. There are mostly three categories: 1) Make sure the shifted type is wide enough. This depends on the type existing, which, for 64 bit, might be a problem with older compilers / include file sets (pre-C99 / C++11), and also with a few ports where providing a 64 integer is not feasible; however, this should generally not be a problem for a contemporary binutils host. 1a) You could use a (unsigned) long long literal, like 1LL or 1ULL . 1b) You could use the difference of two literals that imply a sufficiently large type due to the size of the literals, e.g. (0xdeadbeef1 - 0xdeadbeef0) 1c) You can cast to long long or unsigned long long by naming the type, e.g. (long long)1 . 1d) You can make sure the <stdint.h> header is included and use one of the sufficiently wide types defined there. 2) Split the shift into multiple shifts of smaller shift counts so that a more readily available / cheaper type can be used. 2a) Split into rounded down & rounded up halves to make do with half the type width, e.g. (1UL << (fixP->fx_size * 4)) << (fixP->fx_size * 4) . 2b) Where a shift count value (range) is know, use this to cheaper split. (fixP->fx_size * 8) is in the range [8..32] , so you can subtract one to get it into the range [7..31]. Then you can do the left shift of the literal by 1 first to get a modified literal: (2UL << (fixP->fx_size * 8 - 1) 2c) A combination and/or replication of the above strategies to bridge a wider difference between type width and shift count. 3) Avoid the shift altogether by using an alternate computation, e.g. a table lookup. There is also the further problem that the 'long' type of max is not known to be wide enough to accommodate the value 0xffffffff . You can use unsigned long for that, but then you can't do a defined conversion to a signed type without widening first. And the comparison to val makes no sense when that is 32 bit, either.
(In reply to Joern Rennecke from comment #20) > (In reply to Quentin Boswank from comment #17) > > Created attachment 16107 [details] > > patch for gas bug zeroing 16 or 32 bit width jumptables when relaxing V2 > > max = (1ul << (fixP->fx_size * 8)) - 1; > > There are many ways to avoid shifting by a shiftcount as large or larger than > type bing shifted. This is not one of them. > available / cheaper type can be used. > 2a) Split into rounded down & rounded up halves to make do with half the > type width, > e.g. (1UL << (fixP->fx_size * 4)) << (fixP->fx_size * 4) . > 2b) Where a shift count value (range) is know, use this to cheaper split. > (fixP->fx_size * 8) is in the range [8..32] , so you can subtract one to > get > it into the range [7..31]. Then you can do the left shift of the literal > by 1 first > to get a modified literal: (2UL << (fixP->fx_size * 8 - 1) > 2c) A combination and/or replication of the above strategies to bridge a > wider > difference between type width and shift count. > > 3) Avoid the shift altogether by using an alternate computation, e.g. a > table lookup. > > There is also the further problem that the 'long' type of max is not known > to be > wide enough to accommodate the value 0xffffffff . You can use unsigned long > for that, but then you can't do a defined conversion to a signed type > without widening first. And the comparison to val makes no sense when that > is 32 bit, either. Thats what the if is designed to work around by just disabling the check if we can not fit into the widths.