This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC] Take 2: Adding new files for Interix port


> I'm sorry to be such a nitpicker, but there are several places where
> the use of whitespace in and capitalization of comments is still
> slightly wrong:

Please don't be sorry. It's me who feels bad for still leaving some of
these style errors. I thought I fixed them all, I'll fix the remaining
ones.

> > The nm- and xm- files contains macros that you have never seen so far.
> > They are part of some changes that were made in common parts of the GDB
> > source. They are probably temporary, all the more so as I understand
> > that we want to get rid of these files in the long run anyway.
> 
> I don't think we're going to tolerate those changes in most of the
> common parts of GDB.  Therefore, what's the point of adding them now?

It would make my life simpler: the current interix port still needs some
rework to bring it to a point where all the changes would be acceptable
for integration (albeit with some minor edits, based on the GDB
developers feedback).

But if this is not acceptable, then I will check-in the part that is
approved, and keep this changes localy until I can finally get rid of
them...

> Anyway please use a uniform naming convention for these files; now you
> have xm-i386interix.h and nm-interix.h, and you mention xm-interix.h
> in your ChangeLog.

Ok, will do.

> > /* FIXME: configure should be taught about this. */
> > #define PRINTF_HAS_LONG_LONG 1
> > #ifdef __GNUC__
> > #define BFD_HOST_64_BIT long long
> > #define BFD_HOST_U_64_BIT unsigned long long
> > #elif defined(_MSC_VER)
> > #define BFD_HOST_64_BIT __int64
> > #define BFD_HOST_U_64_BIT unsigned __int64
> > #else
> > #error... OK what compiler is this?
> > #endif
> > 
> > /* FIXME: configure should be taught about this. */
> > #undef LONGEST
> > #define LONGEST BFD_HOST_64_BIT
> > 
> > /* FIXME: configure should be taught about this. */
> > #undef ULONGEST
> > #define ULONGEST BFD_HOST_U_64_BIT
> 
> This should really be addressed in BFD, not GDB.  I suspect that with
> this approach you'll encounter problems when you build a 64-bit
> enabled BFD.

That makes sense. Will do.

Regarding the xm- file:
> > /* Interix has a minimal sbrk() (but not what's wanted for this usage),
> >    and its relationship to environ[] is not what's usually expected
> >    (as in, there is no specific relationship at all). Just pretend we don't
> >    have an sbrk().  */
> > #undef HAVE_SBRK
> > 
> > /*  Used in coffread.c to adjust the symbol offsets.  */
> > #define ADJUST_OBJFILE_OFFSETS(objfile, type) \
> >     pei_adjust_objfile_offsets(objfile, type)
> > 
> > extern CORE_ADDR bfd_getImageBase(bfd *abfd);
> > #define NONZERO_LINK_BASE(abfd) bfd_getImageBase(abfd)
> > 
> > /*  Used in valops.c, to change the name of the malloc function when
> >     trying to allocate memory in the inferior. On Interix, we need to
> >     call _malloc.  Should this be added to the target vector?  */
> > #define NAME_OF_MALLOC "_malloc"
> 
> This doesn't belong here.  Probably should be moved to your nm.h file.

Just to make sure I understand which macros you refer to, would you mind
clarifying for me?
  - HAVE_SBRK  (I assume leaving it in the xm- file is ok, since this
    is where you recommended in a previous message where to put it)
  - ADJUST_OBJFILE_OFFSETS? 
  - NONZERO_LINK_BASE?
  - NAME_OF_MALLOC? I would actually think that this macro should
    be in the -tdep file, since it is the name of function to call
    in the inferior, which lives on the target. What do you think?
    Should it really be in the nm.h file?

> > void
> > supply_gregset (gregset_t *gregsetp)
> > {
> >   int regi;
> >   greg_t *regp = (greg_t *) & gregsetp->gregs;
> 
> Indent gets this wrong :-(.  Please remove the space after the ampersand.

Thanks, fixed.

> > #include <setjmp.h>
> > 
> > static struct core_fns interix_core_fns = {
> 
> Please move the bracket on to the next line.

Thanks, fixed.

> > static int
> > i386_interix_pc_in_sigtramp (CORE_ADDR pc, char *name)
> > {
> >   /* This is sufficient, where used, but is NOT a complete test; There
> >      is more in INIT_EXTRA_FRAME_INFO (a.k.a. interix_back_one_frame) */
> >   return (pc >= tramp_start && pc < tramp_end)
> >     || (pc >= null_start && pc < null_end);
> 
> Please use an extra pair of braces to get the indentation right.

argh, added.

> > static int
> > i386_interix_in_solib_call_trampoline (CORE_ADDR pc, char *name)
> > {
> >   return skip_trampoline_code (pc, name);
> > }
> 
> Didn't you rename skip_trampoline_code?

Yes, I did. And I actually resync'ed  my version of the sources. So
this is already fixed.

> >   return thisframe->signal_handler_caller
> >     || (chain != 0
> > 	&& !inside_entry_file (read_memory_integer (thisframe->frame + 4,
> > 						    4)));
> 
> Again, please use an extra pair of braces.

Braces added.

> >   gdbarch_register_osabi (bfd_arch_i386, GDB_OSABI_INTERIX,
> > 			  i386_interix_init_abi);
> 
> You'll have to add these GDB_OSABI constants to osabi.h first.

Ok.

I will start devoting more time working on bfd, to at least get the
BFD-related comments above solved. I'll then add this GDB_OSABI, and
then resubmit these files, with hopefully all your comments addressed.

Thanks again for the review,
-- 
Joel


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]