Created attachment 15492 [details]
Suggested fix

Attached patch makes NetBSD/landisk (sh4) cross-build complete successfully on NetBSD/amd64, where it used to fail before.  Native gcc built as part of the system (host sh3 -> target sh3) works and produces binaries identical to binaries produced by the native compiler w/out this patch (cross-compiled from i386).

The patch basically defers the choice of L/LL suffix until the moment when insn-*.c is compiled with correct HOST_WIDE* defines (gen*.c uses build machine HOST_WIDE* values, not target machine).

There's still the problem (marked with XXX in the patch) with the minimim host wide decimal constant that I'm not sure how to address properly, short of always generating -1/+1 hack for all negative values.

Does this work with a newer GCC?

The patch above has been committed to the NetBSD imported version of gcc back in April 2008 and has been merged in all the new imported versions ever since.

It's been quite a few years so I don't remember most details, however I assume the patch is still relevant.  I will try to back it out and see what happens.

Seems to fail more or less the same way without the patch.  NetBSD current with gcc 5.4.0.  Building on linux/amd64.  When native gcc is cross compiled

shle--netbsdelf-c++ ... -c insn-emit.c

fails with:

insn-emit.c:6382:3: error: this decimal constant is unsigned only in ISO C90 [-Werror]

This is about 4294967295L in gen_mshflo_l_di().   It no longer complains about -4294967296L in gen_mshfhi_l_di() just above, though.  The relevant fragment of insn-emit.c is:

/* /home/uwe/work/netbsd/ro/src/external/gpl3/gcc/dist/gcc/config/sh/sh.md:15987 */
rtx
gen_mshfhi_l_di (rtx operand0 ATTRIBUTE_UNUSED,
        rtx operand1 ATTRIBUTE_UNUSED,
        rtx operand2 ATTRIBUTE_UNUSED)
{
  return gen_rtx_SET (VOIDmode,
        operand0,
        gen_rtx_IOR (DImode,
        gen_rtx_LSHIFTRT (DImode,
        operand1,
        const_int_rtx[MAX_SAVED_CONST_INT + (32)]),
        gen_rtx_AND (DImode,
        operand2,
        GEN_INT (-4294967296L))));
}

/* /home/uwe/work/netbsd/ro/src/external/gpl3/gcc/dist/gcc/config/sh/sh.md:16037 */
rtx
gen_mshflo_l_di (rtx operand0 ATTRIBUTE_UNUSED,
        rtx operand1 ATTRIBUTE_UNUSED,
        rtx operand2 ATTRIBUTE_UNUSED)
{
  return gen_rtx_SET (VOIDmode,
        operand0,
        gen_rtx_IOR (DImode,
        gen_rtx_AND (DImode,
        operand1,
        GEN_INT (4294967295L)),
        gen_rtx_ASHIFT (DImode,
        operand2,
        const_int_rtx[MAX_SAVED_CONST_INT + (32)])));
}

(In reply to Valeriy E. Ushakov from comment #4)

>         GEN_INT (-4294967296L))));

>         GEN_INT (4294967295L)),

I think those should be -4294967296LL and 4294967295LL.  Do you have that build setup at hand to try this out?

BTW, those two patterns do not exist anymore in trunk, as SH5 stuff has been removed.

(In reply to Oleg Endo from comment #5)
> (In reply to Valeriy E. Ushakov from comment #4)
> 
> >         GEN_INT (-4294967296L))));
> 
> >         GEN_INT (4294967295L)),
> 
> I think those should be -4294967296LL and 4294967295LL.

Right, with the patch above (committed in NetBSD) I get the following diff between insn-emit.c:

-       GEN_INT (-4294967296L))));
+       GEN_INT (HOST_WIDE_INT_CONSTANT (-4294967296)))));

... etc

so, effectively, it removes the L/LL decision from genemit.c and delays it until insn-emit.c is compiled.


> Do you have that build setup at hand to try this out?

I guess the easiest way to get out NetBSD source tree and to cross-build say NetBSD/landisk on a 64 bit host (any linux/amd64 will do, osx probably too, but there were a couple of pitfalls recently).

You can grab sources from e.g. http://ftp.netbsd.org/pub/NetBSD/NetBSD-7.0.1/source/sets/ and consult BUILDING, but in a nutshell something like

./build.sh -m landisk -j8 -u -U distribution

(where -j8 is the amount of parallel jobs you want).
You can mail me privately if you need any help with this.


> BTW, those two patterns do not exist anymore in trunk, as SH5 stuff has been
> removed.

Yes, but that just masks the bug that L/LL selection is done in wrong context.

(In reply to Valeriy E. Ushakov from comment #6)
> (In reply to Oleg Endo from comment #5)
> > BTW, those two patterns do not exist anymore in trunk, as SH5 stuff has been
> > removed.
> 
> Yes, but that just masks the bug that L/LL selection is done in wrong
> context.

OK, so what would be a better way to word the title then, such that it doesn't mention an obsolete target?

I don't understand.  The title mentions sh3, which is not obsolete.

It's been 11 (eleven!) years.  As far as I know this patch (adjusted to catch up with the changes, but essentially the same) is still necessary for gcc 6.4 that is the compiler in NetBSD-current.

(In reply to Valeriy E. Ushakov from comment #8)
> 
> It's been 11 (eleven!) years.  As far as I know this patch (adjusted to
> catch up with the changes, but essentially the same) is still necessary for
> gcc 6.4 that is the compiler in NetBSD-current.

The patch contains changes to things outside the SH backend, so I can't approve it.  Can you please send the most recent version of the patch to the patches ML so that other people can review it?

(In reply to Valeriy E. Ushakov from comment #8)
> I don't understand.  The title mentions sh3, which is not obsolete.
> 

Sorry, I misunderstood the sh numbering system when asking that last night (it was late and I was tired)

Created attachment 44668 [details]
Diff against gcc-6.4.0

This is essentially the same diff except gcc now provides its own HOST_WIDE_INT_C() macro, so the patch uses that instead of defining its own.

I've attached updated patch against gcc 6.4.0.  If I un-apply that patch to the NetBSD tree with patch -R (i.e. revert the files to their original state as in gcc 6.4.0) I get

$ nbmake-landisk insn-emit.o
#   compile  backend/insn-emit.o
/home/uwe/work/netbsd/build/tools/bin/shle--netbsdelf-c++ -Os -freorder-blocks -Wall -Wpointer-arith -Wno-sign-compare -Wa,--fatal-warnings -Wno-uninitialized -Wno-maybe-uninitialized -Werror   -fPIE -Wno-narrowing -Wno-unused -std=gnu++98 -Wno-stack-protector -fno-exceptions -fno-rtti -fasynchronous-unwind-tables   -I. -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/usr.bin/backend/../gcc/arch/sh3el -DIN_GCC -DHAVE_CONFIG_H -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/. -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../include -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libcpp/include -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libdecnumber -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libdecnumber/dpd -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libbacktrace -DTARGET_NAME=\"shle--netbsdelf\" -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/libgcc -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/usr.bin/backend/../../lib/libgcc/libgcov/arch/sh3el --sysroot=/home/uwe/work/netbsd/build/distrib/landisk -DLOCALEDIR=\"/usr/share/locale\" -DNETBSD_NATIVE -I. -DENABLE_SHARED_LIBGCC  -c    insn-emit.c -o insn-emit.o
insn-emit.c:6346:3: error: this decimal constant is unsigned only in ISO C90 [-Werror]
   return gen_rtx_SET (operand0,
   ^~~~~~
cc1plus: all warnings being treated as errors
[...]
$ cat -n insn-emit.c | sed -n '6341,6354p'
  6341  rtx
  6342  gen_mshflo_l_di (rtx operand0 ATTRIBUTE_UNUSED,
  6343          rtx operand1 ATTRIBUTE_UNUSED,
  6344          rtx operand2 ATTRIBUTE_UNUSED)
  6345  {
  6346    return gen_rtx_SET (operand0,
  6347          gen_rtx_IOR (DImode,
  6348          gen_rtx_AND (DImode,
  6349          operand1,
  6350          GEN_INT (4294967295L)),
  6351          gen_rtx_ASHIFT (DImode,
  6352          operand2,
  6353          const_int_rtx[MAX_SAVED_CONST_INT + (32)])));
  6354  }

and

$ nbmake-landisk insn-recog.o
#   compile  backend/insn-recog.o
/home/uwe/work/netbsd/build/tools/bin/shle--netbsdelf-c++ -Os -freorder-blocks -Wall -Wpointer-arith -Wno-sign-compare -Wa,--fatal-warnings -Wno-uninitialized -Wno-maybe-uninitialized -Werror   -fPIE -Wno-narrowing -Wno-unused -std=gnu++98 -Wno-stack-protector -fno-exceptions -fno-rtti -fasynchronous-unwind-tables   -I. -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/usr.bin/backend/../gcc/arch/sh3el -DIN_GCC -DHAVE_CONFIG_H -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/. -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../include -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libcpp/include -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libdecnumber -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libdecnumber/dpd -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/../libbacktrace -DTARGET_NAME=\"shle--netbsdelf\" -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/libgcc -I/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/usr.bin/backend/../../lib/libgcc/libgcov/arch/sh3el --sysroot=/home/uwe/work/netbsd/build/distrib/landisk -DLOCALEDIR=\"/usr/share/locale\" -DNETBSD_NATIVE -I. -DENABLE_SHARED_LIBGCC  -c    insn-recog.c -o insn-recog.o
insn-recog.c:1532:7: error: this decimal constant is unsigned only in ISO C90 [-Werror]
       || XWINT (x4, 0) != -2147483648L
       ^~
insn-recog.c:5762:5: error: this decimal constant is unsigned only in ISO C90 [-Werror]
     case -2147483648L:
     ^~~~
/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/config/sh/sh.md:15774:15: error: this decimal constant is unsigned only in ISO C90 [-Werror]
   DONE;
               ^ 
/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/config/sh/sh.md:4832:17: error: this decimal constant is unsigned only in ISO C90 [-Werror]
       (and:SI (match_dup 1) (const_int 1))))
                 ^~~~
/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/config/sh/sh.md:16430:17: error: this decimal constant is unsigned only in ISO C90 [-Werror]
 (define_split
                 ^   
/home/uwe/work/netbsd/ro/src/external/gpl3/gcc.old/dist/gcc/config/sh/sh.md:14012:15: error: this decimal constant is unsigned only in ISO C90 [-Werror]
   [(set (match_operand:DF 0 "fp_arith_reg_operand" "=f")
               ^~
cc1plus: all warnings being treated as errors

The above build was done on a linux/amd64 host.

The error happens when NetBSD build cross-compiles native NetBSD/sh3 gcc, i.e. the compiler that will run natively on sh3 as part of the NetBSD distribution.

I'm sorry, but I don't have time or energy to come up with a standalone reproducer, especially given prior history of this bug report being ignored for over a decade.  Someone familiar with gcc code base should be able to do that easily I think.  And as I said in comment #5, you can reproduce it with the NetBSD source tree with a single command on about any modern unix on a 64-bit host.

I guess linux-sh folks have never run into this because they use cross-compiler exclusively and never compile stuff on sh3 itself, so they never build native gcc for sh3.

Sorry, I meant comment #6 in the above.

(In reply to Valeriy E. Ushakov from comment #11)
> Created attachment 44668 [details]
> Diff against gcc-6.4.0
> 
> This is essentially the same diff except gcc now provides its own
> HOST_WIDE_INT_C() macro, so the patch uses that instead of defining its own.

can you please send it to the gcc-patches mailing list for review?

(In reply to Eric Gallager from comment #15)
> (In reply to Valeriy E. Ushakov from comment #11)
> > Created attachment 44668 [details]
> > Diff against gcc-6.4.0
> > 
> > This is essentially the same diff except gcc now provides its own
> > HOST_WIDE_INT_C() macro, so the patch uses that instead of defining its own.
> 
> can you please send it to the gcc-patches mailing list for review?

This patch has been sitting in your bugtracker for 11 years.  Anything I know about this bug is written in this bug report and swapped out of my active memory, so I cannot meaningfully answer any questions about that patch on gcc-patches@ other than by referring people to what's written here in this bug report.   Why do I have to go through this strange ritual of taking this patch out of gcc's own bugtracker and sending it to gcc's own list for proposed patches?  This is not some proposed change that I can meaningfully advocate (like a new feature or something).

I.e. what that action is going to change from the standpoint of communication setup?  As I see it, it can only make things worse b/c if I'm actually asked any questions I can't answer them, or someone replies to that patch only to the mailing list and I miss that reply (and it's not recorded here too).  And then it will just look as my fault, not engaging in a proper discussion.

(In reply to Valeriy E. Ushakov from comment #16)
> (In reply to Eric Gallager from comment #15)
> > (In reply to Valeriy E. Ushakov from comment #11)
> > > Created attachment 44668 [details]
> > > Diff against gcc-6.4.0
> > > 
> > > This is essentially the same diff except gcc now provides its own
> > > HOST_WIDE_INT_C() macro, so the patch uses that instead of defining its own.
> > 
> > can you please send it to the gcc-patches mailing list for review?
> 
> This patch has been sitting in your bugtracker for 11 years.  Anything I
> know about this bug is written in this bug report and swapped out of my
> active memory, so I cannot meaningfully answer any questions about that
> patch on gcc-patches@ other than by referring people to what's written here
> in this bug report.   Why do I have to go through this strange ritual of
> taking this patch out of gcc's own bugtracker and sending it to gcc's own
> list for proposed patches?  This is not some proposed change that I can
> meaningfully advocate (like a new feature or something).
> 
> I.e. what that action is going to change from the standpoint of
> communication setup?  As I see it, it can only make things worse b/c if I'm
> actually asked any questions I can't answer them, or someone replies to that
> patch only to the mailing list and I miss that reply (and it's not recorded
> here too).  And then it will just look as my fault, not engaging in a proper
> discussion.

Don't ask me, I didn't create the rule... just trying to help move things along here...

(In reply to Valeriy E. Ushakov from comment #16)
> Why do I have to go through this strange ritual of
> taking this patch out of gcc's own bugtracker and sending it to gcc's own
> list for proposed patches?

I am knowledge-free about this particular area, just wanted to make
some general comments.

Several reasons, some of them overlapping.

This is how gcc development operates.  For a large project like gcc
with many volunteers, you need to have rules, and in general, people
should stick to them.

Posting to gcc-patches gives everybody a chance to look at patches
and speak up if they have any concerns or advice.  Not very many
people read individual PRs.

Finally, it's a very good thing that you need approval from somebody.
Compilers are notoriously complex beasts, and regressions for some
corner case can creep in quite easily - less easily if somebody
else has a look.

Of course, another thing that can be done is to add a maintainer for
a particular area to the PR. I have done so with this.

Let me take a look into fix this issue for GCC 15. It might be already fixed when GCC moved over to requiring a C++11 compiler ...

So sh5 support was removed in r7-339-ge1fab8ba2337fd and that removed the use of 4294967296. So it is hard to test something which does not exist any more.

So closing as won't fix for GCC 7. When someone needs -4294967296 this will come up again but right now there is no need to fix this.