Re: [RFC] [1/2] First cut at multi-executable support.

[Pedro Alves <pedro at codesourcery dot com>]

On Wednesday 17 June 2009 19:19:23, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> All-in-all, what do you think of this, and the patch below?  
> 
> I think it is unbelievably awesome.
> 
> Pedro> There's certainly scope of a lot of goodies to be added on top
> Pedro> of this, like extending thread control support (threads groups, itsets
> Pedro> or whatever); better support for symbol scoping a-la HPD's ## syntax
> Pedro> would be great too; Making the breakpoints module smarter about
> Pedro> that also would be nice.
> 
> ... sharing objfiles across inferiors would be nice.  It seems to me
> that this would be important for capping memory use when there are
> many inferiors.  I wonder if this makes sense with the aspace/sspace
> stuff, given that an objfile now has a symbol space.

We could split an objfile in two, with the shared parts not having a
reference to a symbol space.  I can see us sharing the type stuff as
a first step.  The min|p|symbol,blocks,etc (everything that holds
an address) is a different story due to the fact that objfiles are
relocated in place.  Ideally, I think we would
stop relocating objfiles' symbols etc. in place (objfile_relocate),
and instead store unrelocated (with VMAs == link addresses) addresses in
the symbols, and then be able to given a symbol and a sspace/inferior,
request its "cooked" address, that is, we'd store section relocation
offsets on a separate structure, and apply then on demand for
the space we're interested in.  E.g:.

struct objfile
{
   minsyms;
   psymtabs/psymbols /* (while we have them) */
   symtab/symbols;
   section_offsets; /* link time offsets (or unique offsets arranged by GDB if
                       this is a relocatable object */
   num_sections;
};

struct cooked_objfile
{
   struct objfile *raw_obfile;
   struct symbol_space *sspace;

   section_offsets; /* load time offsets (relocation offsets), 
                       array the size of raw_objfile->num_sections */
};

Then you have the reverse side of the story, that is, in many, many
places, you have a PC, and you want the matching symbol.  Here, I think
we should look for the matching cooked section for the PC, uncook this
PC (unrelocate it, given it section offset), and then do the symbol
lookup with this uncooked PC.

At a high distance, this sounds feasible to me, but, looking at the
code, this looks like a huge effort, and I'm sure I'm missing a lot
of complications.  OTOH, having this would help make the
breakpoints module much smarter and leaner.  E.g., a new inferior
loaded the same objfile where you already have a breakpoint set?
Given the breakpoint symbol, get the cooked address in this new
address space, and set a new raw breakpoint location there, no need
to re-evalutate the breakpoint's string expression in the new space.

> 
> When gdb sees a new inferior, does it immediately read its debuginfo?

About the same as if you had started the new inferior with
'gdb newinferiorfile -ex "run"'.

> This seems like another possible performance problem; lazily reading
> it would be friendlier.  In a scenario like the "make" case, I would
> assume that most inferiors will not actually require any user
> attention, and time and memory spent on their debuginfo is just
> wasted.

Yeah, I know you have patches for this.  ;-)

In the "make" case, you're not likely to have debug info
for "make", "sed", "cut", "awk", "true", "false", whatnot, though.
But yeah, there's still a lot that can be done.

> 
> 
> I saw this in the output:
> 
>     [Thread debugging using libthread_db enabled]
>     process 16875 is executing new program: /bin/true
>     (no debugging symbols found)
>     (no debugging symbols found)
>     (no debugging symbols found)
> 
>     Program exited normally.
>     make[2]: Leaving directory `/home/pedro/gdb/sspaces/build/gdb/gdbserver'
> 
>     Program exited normally.
> 
>     Program exited normally.
> 
>     Program exited normally.
>     make[1]: Leaving directory `/home/pedro/gdb/sspaces/build/gdb'
> 
>     Program exited normally.
> 
> I think the "(no debugging symbols found)" thing has been addressed.
> At least, there was a patch.
> 
> "Program exited normally." could also use some love... at least some
> info about the program, and maybe removing the excess newlines?

For my own testing, I've tweaked those messages to include the
process id.  It's just mad otherwise.  However, it has been
one of my goals to not change much how the single-inferior case
works/outputs as a first incremental step.  I'm sure there will
be different opinions on to what GDB should output, so this avoids
such discussions for now :-).  In this case, in truth, I deferred
proposing to change such outputs until afterwards, to avoid having
to go and do a bunch of mechanical testsuite patching.  I certainly
would like to have better messages!

This reasoning holds also for other things I think we'll want
to change in the future, like for example, the follow-fork model
(new inferior only pops up after resuming, not when the fork is
caught).

> 
> 
> I see in many places that we pass an address_space and a CORE_ADDR
> together, e.g.:
> 
> +insert_single_step_breakpoint (struct address_space *aspace, CORE_ADDR next_pc)
> 
> Does a CORE_ADDR make sense without an address_space?

Sure does.  In many places, you only need to know the offset
into the symbol/address space (that's a CORE_ADDR), while the space
is implied from context.

> I'm curious whether you considered putting the address_space into
> CORE_ADDR, 

For 5 minutes only.  This is C, not C++.  :-)  

>grep CORE_ADDR * -rn | grep -v ChangeLog | wc -l
4470

What we have is an address or symbol space (the new entities),
and an offset into them (CORE_ADDR).  It just happens that
GDB is (mostly anyway, and also discarding the unused bit hacks)
prepared currently for a single linear address space, with CORE_ADDRess
representing offsets into this single address space.

I do think there's scope in the future (particularly if we add
sharing of objfiles like I mentioned above), for an object
representing an address like:

sspace | objfile | section | offset_in_section
                    index           ^ scalar


> or making a new type holding both. 

I just didn't find a use for that, that's all.

> 
> 
> +#define exec_bfd current_symbol_space->ebfd
> +#define exec_bfd_mtime current_symbol_space->ebfd_mtime
> 
> +#define so_list_head current_symbol_space->so_list
> 
> +#define symfile_objfile current_symbol_space->symfile_objfile_1
> 
> +#define object_files current_symbol_space->objfiles
> 
> +#define current_target_sections (&current_symbol_space->target_sections)
> 
> I don't mind this kind of thing as a transitional change, to make it
> so a patch doesn't completely explode.  But I think I'd prefer, in the
> long run, to replace these macros with their expansions (or maybe
> accessor macros taking current_symbol_space as an argument)
> everywhere.  What do you think?

Agreed.

> Also, I am curious about the relationship between the current inferior
> and the current symbol space.  I guess whenever the current inferior
> changes, the current inferior space must as well?

Yes, they go hand in hand, most of the times.  We switch the current
space whenever GDB switches current thread (switch_to_thread, for
example).

> 
> +  ui_out_table_header (uiout, 1, ui_left, "current", "");
> 
> For some reason I did not think of leaving the header empty when I did
> this for "info inferiors".  I like this better... since you're also
> touching print_inferior, how about just making that change?

But I had already.  :-)  Or, do you mean to split that change out
of the patch?  I can do that.  I think that would be good idea.

I'm now working on cleaning up a bit the patch and fixing the
random crashes I mentioned, and I'll be posting an updated
patch soon.

-- 
Pedro Alves