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]

Re: [RFA]: Add misc_obstack to object files.


I'd like to throw my hat in the ring, too.  I spent a whole weekend 
several months ago changing the type of all the section offset tables.
I wound up with a 3000 line patch that I didn't even try to get
approved.  (I was working on PR gdb/29, "gdb has fixed size MAX_SECTIONS").
So I care about section offset tables.

> I'm just adding a bin for colored paper, so people will put that stuff
> in there instead in the first place, and it will be plainly clear that
> they are supposed to be doing so.

So you don't have a manifestation of a bug in your hand; rather, you are
cleaning up something that is broken.  That's fine.  I am comfortable
with your rationale, now that I know what it is.

I see three specific problems:

. in lines 1984-1993 of symfile.c, you change the "psymtab" pointer from
  psymbol_obstack to misc_obstack.  It's not obvious why this structure
  is on misc_obstack.  Since it's not obvious, a comment would help.

. as Elena mentioned, there are other files which allocate section
  offsets on the psymbol_obstack.  I see somread.c in particular.
  
  Right now it's difficult to test somread.c, because even if you have
  access to hppa hardware, FSF gdb does not link for that configuration
  (PR gdb/63).  If you can't test it, and you don't want to go for a
  blind patch to somread.c, then I think that a FIXME comment would be
  appropriate, to explain to the next hpux maintainer why this target
  allocates section_offsets on psymbol_objstack when other targets use
  misc_obstack.  Something like: "FIXME: this should be misc_obstack,
  same as symfile.c, but I can't test this file so I'm leaving it as
  psymbol_obstack, which ought to continue working for now".

. the new comment on lines 121-137 of objfiles.c is not true yet,
  even after applying your patch.  For example, dbxread.c puts
  DBX_STRINGTAB on psymbol_obstack.  So I challenge your claim
  "All the stuff in objfile that was on the psymbol obstack,
  but didnt' belong, is in the misc obstack ...".

> The only thing even related to psymtabs there is the fact that they
> psymtab structure itself is allocated on the misc obstack, since it
> isn't a partial symbol either. It's a partial symbol table structure,
> containing pointers to partial symbols. 

If you put this in a comment, that would answer my first question
(which might be the same as Elena's question about an extraneous patch
fragment, but I don't know what fragment Elena is referring to).

> I could do them all now if it helps, but it'll make the patch bigger.

I like it better this way, one structure at a time.  I think that
Elena's looking for some sense of whether this is a standalone patch
or part of a bigger series -- not one big patch.

Here are the reasons that I prefer series:

. It's easier to proof-read one concept at a time.

. It's easier to read the first "proof of concept" when there's only
  one structure in it, rather than old structures.

. If something breaks in the future, and I need to binary-search old
  versions of gdb, this improves the granularity that I can resolve the
  bug too.  This is important for symbol table patches as a bug can
  manifest subtly (e.g. lookup_symbol bug -> infinite loop in "maint"
  command when running on an inferior copy of gdb).

Basically there are two different ideas here.  First there is the idea of
what should actually be in gdb.  I'm comfortable with your vision there.
Second there is the idea of structuring one's work so that other people
can deduce that it actually implements the concept (including checking
for implementation bugs).

Just my two cents.  I'll try not to write again, as Elena is the
maintainer here.

Michael


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