Mail Archives: djgpp-workers/2011/07/30/08:21:00
> 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.
- Raw text -