X-Authentication-Warning: delorie.com: mail set sender to djgpp-workers-bounces using -f
X-Recipient: djgpp-workers AT delorie DOT com
Date: Sat, 30 Jul 2011 15:21:03 +0300
From: Eli Zaretskii <eliz AT gnu DOT org>
Subject: Re: [PATCH] allow 64 bit host tools when cross compiling
In-reply-to: <CAA2C=vBtiNnx1XPVHw_6Er1uf0_biO3vpaBEa0zeYQuZKh5BxA@mail.gmail.com>
To: djgpp-workers AT delorie DOT com
Message-id: <83mxfwc634.fsf@gnu.org>
MIME-version: 1.0
Content-type: text/plain; charset=iso-8859-1
X-012-Sender: halo1 AT inter DOT net DOT il
References: <CAA2C=vCTyDxWBgYzoBB31=hoOBHJhL+famn_6XL2eYb3a_3egA AT mail DOT gmail DOT com> <83zkjwcknb DOT fsf AT gnu DOT org> <CAA2C=vBJEgnGapVsNwy7xW9HUCy0Rpxis2P5=05mxBze_qn7vQ AT mail DOT gmail DOT com> <83wrf0chf4 DOT fsf AT gnu DOT org> <CAA2C=vBtiNnx1XPVHw_6Er1uf0_biO3vpaBEa0zeYQuZKh5BxA AT mail DOT gmail DOT com>
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from QUOTED-PRINTABLE to 8bit by delorie.com id p6UCKvsm020962
Reply-To: djgpp-workers AT delorie DOT com
Errors-To: nobody AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk

> Date: Sat, 30 Jul 2011 14:34:00 +0300
> From: Ozkan Sezer <sezeroz AT gmail DOT com>
> 
> [1:text/plain Hide]
> On Sat, Jul 30, 2011 at 11:16 AM, Eli Zaretskii <eliz AT gnu DOT org> wrote:
> >> Date: Sat, 30 Jul 2011 10:17:43 +0300
> >> From: Ozkan Sezer <sezeroz AT gmail DOT com>
> >>
> >> >   IOW, what is the purpose of this compilation?
> >>
> >> To compile the djgpp runtime library
> >
> > Is dxegen required to be compiled and run natively for that?  (Sorry,
> 
> Yes, it is.
> 
> > I have long forgotten the details of the DJGPP build process.)  If it
> > needs to be compiled and run natively, then I understand the need for
> > the changes; otherwise I do not.
> >
> 
> Why do you think I cared and posted a patch here anyway??!
> 
> (Can someone who actually does compile his own stuff confirm
> this to him?)

Please cool down.  There was no intent to offend you in any way.
Explaining the purpose of your changes is an integral part of the
patch review process, so I don't think I asked anything out of line.
Last time I worked on these issues was a few years ago; I did
apologize for not remembering the details.

> >  #if _LP64
> >  # define LONG int
> >  # define ULONG unsigned int
> >  #else
> >  # define LONG long
> >  # define ULONG unsigned long
> >  #endf
> >
> > This will keep the x86_64 compiler happy, but OTOH will not change
> > what the 32-bit compiler sees at all, thus avoiding various warnings
> > about type mismatch.
> 
> I dislike such hacks when we have a legitimate solution of stdint.h,
> but won't argue much about it.  Did a similar change in the attached
> patch.

Thank you.  I have a few more comments:

> +#ifndef _DJ_DEFINED_NATIVE_TYPES
> +#define _DJ_DEFINED_NATIVE_TYPES

Why are these additional macros needed?

>  struct external_lineno {
>  	union {
> -		unsigned long l_symndx __attribute__((packed));	/* function name symbol index, iff l_lnno == 0 */
> -		unsigned long l_paddr __attribute__((packed));		/* (physical) address of line number */
> +		ULONG32 l_symndx;	/* function name symbol index, iff l_lnno == 0 */
> +		ULONG32 l_paddr;		/* (physical) address of line number */
>  	} l_addr;
>  	unsigned short l_lnno;						/* line number */
> -};
> +} __attribute__((packed));

Why did you remove the `__attribute__((packed))' parts (here and
elsewhere)?  These structures must be binary-compatible with data
produced by utilities outside of DJGPP control, so letting the
compiler pad the struct members to its liking might cause subtle bugs.

>  #define DXE_LD "ld"
> -#include <sys/dxe.h>
> -#include <coff.h>
>  #else
>  /* Linux violates POSIX.1 and defines this, but it shouldn't.  We fix it. */
>  #undef _POSIX_SOURCE
> +#endif
> +
> +/* Always include local copies of sys/dxe.h and coff.h because of
> + * NATIVE_TYPES stuff. */
>  #include "../../include/sys/dxe.h"
>  #include "../../include/coff.h"
> -#endif

Hmm... I'd rather fix the -I command-line arguments to the compiler
when it compiles dxe3gen.c, instead of this.  I think it's cleaner.
Can that be done?

> -#define VALID_RELOC(r) ((r.r_type != RELOC_REL32) && (r.r_symndx != -1UL))
> +#define VALID_RELOC(r) ((r.r_type != RELOC_REL32) && (r.r_symndx != 0xFFFFFFFF))

I think this is cleaner:

 #define VALID_RELOC(r) ((r.r_type != RELOC_REL32) && (r.r_symndx != (ULONG32)-1))

> -               relocs[j].r_symndx = -1UL;
> +               relocs[j].r_symndx = 0xFFFFFFFF;

Same here.