Mail Archives: djgpp-workers/2011/07/30/08:36:57
On Sat, Jul 30, 2011 at 3:21 PM, Eli Zaretskii <eliz AT gnu DOT org> wrote:
>> 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.
>
OK
>> > #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?
>
The types are defined in two different headers, so the guard
prevents multiple definition
>> 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.
>
How is 'long' packed?? The intention there must be
to pack the structure, not the type itself. In the linux
version (linux/coff.h, which this theader is derived off)
all members are char arrays, so there are no issues
in there. But here, you must pack the structure itself
to prevent compiler alighing the middle members to
its own liking.
>> #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?
>
I actually made dxe3gen the same as dxegen.c here. I don't want
to mess with the makefiles for now. It can be done, however it must
be done at multiple places, AFAIR.
>> -#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.
Well, is (LONG32)-1 not the same as 0xffffffff ?
But feel free to edit.
Regards.
--
O.S.
- Raw text -