build error for mn10300-elf sim with your recent commit

build error for mn10300-elf sim with your recent commit

[Hans-Peter Nilsson]

My autotester complains that the sim build for mn10300-elf is
broken, and your entry was the only one at the top of
sim/mn10300/ChangeLog.  There was a working build 5-6 hours
earlier:

gcc -DHAVE_CONFIG_H     -DPROFILE=1 -DWITH_PROFILE=-1   -DWITH_ALIGNMENT=NONSTRICT_ALIGNMENT -DWITH_TARGET_WORD_BITSIZE=32 -DWITH_TARGET_WORD_MSB=31 -DWITH_TARGET_BYTE_ORDER=LITTLE_ENDIAN   -DWITH_HW=1 -DWITH_HOST_BYTE_ORDER=LITTLE_ENDIAN    -DWITH_RESERVED_BITS=1    -Wall -Wdeclaration-after-statement -Wpointer-arith -Wformat-nonliteral -Wno-pointer-sign -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wmissing-prototypes    -DPOLL_QUIT_INTERVAL=0x20   -I. -I/tmp/hpautotest-sim/src/sim/mn10300 -I../common -I/tmp/hpautotest-sim/src/sim/mn10300/../common -I../../include -I/tmp/hpautotest-sim/src/sim/mn10300/../../include -I../../bfd -I/tmp/hpautotest-sim/src/sim/mn10300/../../bfd -I../../opcodes -I/tmp/hpautotest-sim/src/sim/mn10300/../../opcodes  -g -O2 -c -o interp.o -MT interp.o -MMD -MP -MF .deps/interp.Tpo /tmp/hpautotest-sim/src/sim/mn10300/interp.c
In file included from /tmp/hpautotest-sim/src/sim/mn10300/interp.c:7:
/tmp/hpautotest-sim/src/sim/mn10300/../../bfd/sysdep.h:27:2: error: #error sysdep.h must be included in lieu of config.h
/tmp/hpautotest-sim/src/sim/mn10300/interp.c:618: warning: no previous prototype for 'round_64'
/tmp/hpautotest-sim/src/sim/mn10300/interp.c:646: warning: no previous prototype for 'fpu_status_ok'
make[3]: Leaving directory `/tmp/hpautotest-sim/mn10300-elf/sim/mn10300'
make[2]: Leaving directory `/tmp/hpautotest-sim/mn10300-elf/sim'
make[1]: Leaving directory `/tmp/hpautotest-sim/mn10300-elf'
make[3]: *** [interp.o] Error 1

brgds, H-P

Re: build error for mn10300-elf sim with your recent commit

[Joel Brobecker]

> My autotester complains that the sim build for mn10300-elf is
> broken, and your entry was the only one at the top of
> sim/mn10300/ChangeLog.  There was a working build 5-6 hours
> earlier:

Yeah, I think that might be me. I'll try to reproduce and fix.
If it's an easy fix, I won't be offdended if you send a patch :-).

-- 
Joel

Re: build error for mn10300-elf sim with your recent commit

[Joel Brobecker]

> > My autotester complains that the sim build for mn10300-elf is
> > broken, and your entry was the only one at the top of
> > sim/mn10300/ChangeLog.  There was a working build 5-6 hours
> > earlier:
> 
> Yeah, I think that might be me. I'll try to reproduce and fix.
> If it's an easy fix, I won't be offdended if you send a patch :-).

I think this one is really going to hurt. Amazing how a little change
in bfd can have ripple effects everywhere.

Basically: BFD changed the inclusion rules, now causing a build error
if you include "bfd.h" without having included "config.h" before.
We were including "config.h", but BFD failed to notice because of
the fact that the PACKAGE macro was missing. I added it, and now
we trip another requirement: Either we include "config.h", or we
include "sysdep.h" (which in turn includes "config.h" for you).

sysdep.h provides some definitions for the system that might be
missing some features, like:

    #if !HAVE_DECL_FFS
    extern int ffs (int);
    #endif

So, my first suggestion is to replace all includes of "config.h"
by includes of "sysdep.h". Or rather, I'd create a file similar
to GDB's defs.h, and add a rule that every file should include
that file first.

I'm copying Mike for guidance.  Right now, there are exactly
100 files that include "config.h", for 8 files that include
sysdep.h.

I am also wondering why BFD has this requirement at all. Looking
at the code, there wouldn't be any harm that I could think of if
config.h was included first, and then sysdep.h, since the first
thing sysdep.h does is include config.h.  And if someone includes
sysdep.h first, and then config.h, the second include will be a noop.
Perhaps we should ask the binutils folks as well? Or maybe I should
do some archeology, but I am running out of time for today.

-- 
Joel

Re: build error for mn10300-elf sim with your recent commit

[Pedro Alves]

On 06/15/2012 08:15 PM, Joel Brobecker wrote:

>>> My autotester complains that the sim build for mn10300-elf is
>>> broken, and your entry was the only one at the top of
>>> sim/mn10300/ChangeLog.  There was a working build 5-6 hours
>>> earlier:
>>
>> Yeah, I think that might be me. I'll try to reproduce and fix.
>> If it's an easy fix, I won't be offdended if you send a patch :-).
> 
> I think this one is really going to hurt. Amazing how a little change
> in bfd can have ripple effects everywhere.
> 
> Basically: BFD changed the inclusion rules, now causing a build error
> if you include "bfd.h" without having included "config.h" before.
> We were including "config.h", but BFD failed to notice because of
> the fact that the PACKAGE macro was missing. I added it, and now
> we trip another requirement: Either we include "config.h", or we
> include "sysdep.h" (which in turn includes "config.h" for you).


I'm confused.  Nothing outside of bfd should be including bfd/sysdep.h.
Is that what is happening?

> 
> sysdep.h provides some definitions for the system that might be
> missing some features, like:
> 
>     #if !HAVE_DECL_FFS
>     extern int ffs (int);
>     #endif

>

> So, my first suggestion is to replace all includes of "config.h"
> by includes of "sysdep.h". Or rather, I'd create a file similar
> to GDB's defs.h, and add a rule that every file should include
> that file first.
> 
> I'm copying Mike for guidance.  Right now, there are exactly
> 100 files that include "config.h", for 8 files that include
> sysdep.h.
> 
> I am also wondering why BFD has this requirement at all. Looking
> at the code, there wouldn't be any harm that I could think of if
> config.h was included first, and then sysdep.h, since the first
> thing sysdep.h does is include config.h.  And if someone includes
> sysdep.h first, and then config.h, the second include will be a noop.
> Perhaps we should ask the binutils folks as well? Or maybe I should
> do some archeology, but I am running out of time for today.


-- 
Pedro Alves

Re: build error for mn10300-elf sim with your recent commit

[Joel Brobecker]

> I'm confused.  Nothing outside of bfd should be including bfd/sysdep.h.
> Is that what is happening?

Yep.

And for the record, I did try to compile without including sysdep.h,
and it built just fine. But I am assuming that there are some hosts
that this is going to break, and since I can't test the change on
every single hosts out there...

-- 
Joel

Re: build error for mn10300-elf sim with your recent commit

[Pedro Alves]

On 06/15/2012 08:52 PM, Joel Brobecker wrote:

>> I'm confused.  Nothing outside of bfd should be including bfd/sysdep.h.
>> Is that what is happening?
> 
> Yep.
> 
> And for the record, I did try to compile without including sysdep.h,
> and it built just fine. But I am assuming that there are some hosts
> that this is going to break, and since I can't test the change on
> every single hosts out there...

Only one way to find out.  :-)

$ cd sim
$ find . -name "*.[hc]" | xargs grep sysdep.h
./d10v/interp.c:#include "sysdep.h"
./cr16/interp.c:#include "sysdep.h"
./moxie/interp.c:#include "sysdep.h"
./mcore/interp.c:#include "sysdep.h"
./sh64/sh-desc.c:#include "sysdep.h"
./mn10300/interp.c:#include "sysdep.h"
./microblaze/interp.c:#include "sysdep.h"
./cris/traps.c:/* From ld/sysdep.h.  */
./cris/cris-desc.c:#include "sysdep.h"

One's a comment.  And,

$ find . -name sysdep.h
./moxie/sysdep.h
./mcore/sysdep.h
./microblaze/sysdep.h

So that leaves:

./d10v/interp.c:#include "sysdep.h"
./cr16/interp.c:#include "sysdep.h"
./sh64/sh-desc.c:#include "sysdep.h"
./mn10300/interp.c:#include "sysdep.h"
./cris/cris-desc.c:#include "sysdep.h"

I don't see these ports really caring for hosts other than
GNU/Linux and Windows.

It is wrong to include bfd/sysdeps.h, practically as much as it
is wrong to include bfd/config.h.  bfd/sysdeps.h HAVE_FOOs depend on
bfd's own autoconfigury, not the sim's.

Consolidating all the commonality between all these sysdep.h files
sound nice, though I'm not sure how much work that is.

-- 
Pedro Alves

Re: build error for mn10300-elf sim with your recent commit

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1730 bytes --]

On Friday 15 June 2012 17:14:36 Pedro Alves wrote:
> $ cd sim
> $ find . -name "*.[hc]" | xargs grep sysdep.h
> ./d10v/interp.c:#include "sysdep.h"
> ./cr16/interp.c:#include "sysdep.h"
> ./moxie/interp.c:#include "sysdep.h"
> ./mcore/interp.c:#include "sysdep.h"
> ./sh64/sh-desc.c:#include "sysdep.h"
> ./mn10300/interp.c:#include "sysdep.h"
> ./microblaze/interp.c:#include "sysdep.h"
> ./cris/traps.c:/* From ld/sysdep.h.  */
> ./cris/cris-desc.c:#include "sysdep.h"
> 
> One's a comment.  And,
> 
> $ find . -name sysdep.h
> ./moxie/sysdep.h
> ./mcore/sysdep.h
> ./microblaze/sysdep.h
> 
> So that leaves:
> 
> ./d10v/interp.c:#include "sysdep.h"
> ./cr16/interp.c:#include "sysdep.h"
> ./sh64/sh-desc.c:#include "sysdep.h"
> ./mn10300/interp.c:#include "sysdep.h"
> ./cris/cris-desc.c:#include "sysdep.h"
> 
> I don't see these ports really caring for hosts other than
> GNU/Linux and Windows.
> 
> It is wrong to include bfd/sysdeps.h, practically as much as it
> is wrong to include bfd/config.h.  bfd/sysdeps.h HAVE_FOOs depend on
> bfd's own autoconfigury, not the sim's.
> 
> Consolidating all the commonality between all these sysdep.h files
> sound nice, though I'm not sure how much work that is.

i'd wager that it's more of a copy & paste error between seems.  one of the 
really old ones had an interp.c that included sysdep.h, and when someone 
started a new port, they copied an existing one.  the local sysdep.h wasn't 
needed, but the build still worked, so no one looked further.  the sim code 
base isn't exactly a shining glory of how things should be done :).

i vote for deleting all sysdep.h inclusions where the port doesn't have a 
local sysdep.h.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: build error for mn10300-elf sim with your recent commit

[Hans-Peter Nilsson]

> From: Mike Frysinger <vapier@gentoo.org>
> Date: Sat, 16 Jun 2012 06:55:49 +0200

> i vote for deleting all sysdep.h inclusions where the port doesn't have a
> local sysdep.h.

Sounds right, except that in some cases (like for cris-desc.c)
it's the (CGEN) generator templates files that need fixing.

brgds, H-P

Re: build error for mn10300-elf sim with your recent commit

[Hans-Peter Nilsson]

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Sat, 16 Jun 2012 07:09:27 +0200

> > From: Mike Frysinger <vapier@gentoo.org>
> > Date: Sat, 16 Jun 2012 06:55:49 +0200
> 
> > i vote for deleting all sysdep.h inclusions where the port doesn't have a
> > local sysdep.h.
> 
> Sounds right, except that in some cases (like for cris-desc.c)
> it's the (CGEN) generator templates files that need fixing.

But the failing one was all human-written code.  Fixed thus,
committed.  People, when you edit these things, please
test-build the affected simulators.  It doesn't catch the
weirdest host cases, but it does catch most breakages.

sim/mn10300:

	* interp.c: Include config.h first.  Do not include sysdep.h.

Index: interp.c
===================================================================
RCS file: /cvs/src/src/sim/mn10300/interp.c,v
retrieving revision 1.8
diff -p -u -r1.8 interp.c
--- interp.c	16 Feb 2012 23:17:27 -0000	1.8
+++ interp.c	17 Jun 2012 18:55:44 -0000
@@ -1,10 +1,10 @@
+#include "config.h"
 #include <signal.h>
 
 #include "sim-main.h"
 #include "sim-options.h"
 #include "sim-hw.h"
 
-#include "sysdep.h"
 #include "bfd.h"
 #include "sim-assert.h"
 
brgds, H-P

Re: build error for mn10300-elf sim with your recent commit

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1394 bytes --]

On Saturday 16 June 2012 00:55:49 Mike Frysinger wrote:
> i vote for deleting all sysdep.h inclusions where the port doesn't have a
> local sysdep.h.

i've committed this to fix moxie build:

2012-06-17  Mike Frysinger  <vapier@gentoo.org>

	* interp.c: Include config.h first.  Also include fcntl.h directly.

--- moxie/interp.c	4 Jan 2012 08:28:21 -0000	1.13
+++ moxie/interp.c	17 Jun 2012 23:32:46 -0000
@@ -17,6 +17,8 @@ GNU General Public License for more deta
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#include "config.h"
+#include <fcntl.h>
 #include <signal.h>
 #include <stdlib.h>
 #include "sysdep.h"

then i did this for fun:

2012-06-17  Mike Frysinger  <vapier@gentoo.org>

	* interp.c: Include config.h first.  Also include stdlib.h and
	string.h so we can drop the sysdep.h include.

--- cr16/interp.c	24 May 2012 16:51:41 -0000	1.9
+++ cr16/interp.c	17 Jun 2012 23:29:35 -0000
@@ -17,8 +17,10 @@
    You should have received a copy of the GNU General Public License 
    along with this program. If not, see <http://www.gnu.org/licenses/>.  */
 
+#include "config.h"
 #include <signal.h>
-#include "sysdep.h"
+#include <stdlib.h>
+#include <string.h>
 #include "bfd.h"
 #include "gdb/callback.h"
 #include "gdb/remote-sim.h"
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: build error for mn10300-elf sim with your recent commit

[Joel Brobecker]

> So that leaves:
> 
> ./d10v/interp.c:#include "sysdep.h"
> ./cr16/interp.c:#include "sysdep.h"
> ./sh64/sh-desc.c:#include "sysdep.h"
> ./mn10300/interp.c:#include "sysdep.h"
> ./cris/cris-desc.c:#include "sysdep.h"

OK - I just sent a patch for d10v. cr16 and mn10300 were fixed by
Mike and HP (thank you!).

As for sh64 and cris, the files ars generated by CGEN as explained
earlier in this thread. I just spent the better of 2 hours trying
to regenerate them, to no avail because . Thanks to Doug's help,
I got to the point where cgen tries to regenerate the files, but
fails - and I can't really tell what the problem is. The fix would
need to be in cgen anyway, and I am not sure if it wouldn't affect
the tools other than the sim.

Since these two simulators build fine for now, I would like to
leave it at that. My suggestion is for the maintainers of those
sims to create a local sysdep.h file that imports "config.h" and
possibly other system includes.

-- 
Joel

Re: build error for mn10300-elf sim with your recent commit

[Hans-Peter Nilsson]

> From: Joel Brobecker <brobecker@adacore.com>
> Date: Fri, 15 Jun 2012 21:15:16 +0200

> > > My autotester complains that the sim build for mn10300-elf is
> > > broken, and your entry was the only one at the top of
> > > sim/mn10300/ChangeLog.  There was a working build 5-6 hours
> > > earlier:
> > 
> > Yeah, I think that might be me. I'll try to reproduce and fix.
> > If it's an easy fix, I won't be offdended if you send a patch :-).
> 
> I think this one is really going to hurt. Amazing how a little change
> in bfd can have ripple effects everywhere.
> 
> Basically: BFD changed the inclusion rules, now causing a build error
> if you include "bfd.h" without having included "config.h" before.

Is it really that?  My first thought was that it's *not* that.

All the other simulators I test (arm-elf, cris-elf, d10v-elf,
frv-elf, h8300-elf, iq2000-elf, m32c-elf, m32r-elf, m68hc11-elf,
mcore-elf, mips-elf, powerpc-eabisim, sh-elf, v850-elf) are ok
(fixed since the big config.h breakage), it's just mn10300-sim
that broke now, so I can't help but thinking it's likely a
simple fix.

brgds, H-P