Re: Failures with exelib.exp testcase (was Re: minutes 2010-08-19)

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2011-03-17 at 14:35 +0530, K.Prasad wrote:
> On Fri, Mar 04, 2011 at 04:06:33PM +0100, Mark Wielaard wrote:
> > On Fri, 2011-03-04 at 12:43 +0530, K.Prasad wrote:
> > > Index: systemtap/translate.cxx
> > > ===================================================================
> > > --- systemtap.orig/translate.cxx
> > > +++ systemtap/translate.cxx
> > > @@ -5018,7 +5018,30 @@
> > >  		    || sym.st_value >= end	// beyond current module,
> > >  		    || sym.st_value < base))	// before first section.
> > >              {
> > > -              Dwarf_Addr sym_addr = sym.st_value;
> > > +              Dwarf_Addr sym_addr;
> > > +#ifdef __powerpc__
> > 
> > You shouldn't depend on this, do a check against the e_machine type of
> > the elf file header to check that is it a ppc64 one.
> >
> 
> Done.
>  
> > > +//              Elf64_Addr opd_addr;
> > > +              Dwarf_Addr bias;
> > > +              Elf_Data *data = NULL;
> > > +              Elf_Data in, out;
> > > +              Elf_Scn *opd = dwfl_module_address_section (m, &sym_addr, &bias);
> > > +              Elf* elf = (dwfl_module_getelf (m, &bias));
> > > +
> > > +              if (opd != NULL)
> > > +                {
> > > +                  data = elf_rawdata (opd, NULL);
> > > +                  if (data == NULL)
> > > +                      return DWARF_CB_ABORT;
> > > +                }
> > > +              out.d_buf = *userdata;
> > 
> > userdata is a pointer to the (unused) unwindsym_dump_context pointer
> > provided to us as dwfl_getmodules callback, why are you using it here?
> > Don't you want to store the result in the opd_addr?
> > 
> 
> Ok. Making it "out.d_buf = (void *)&opd_addr".
> 
> > > +              in.d_buf = (char *) data->d_buf + sym_addr;
> > > +              out.d_size = in.d_size = sizeof (Elf64_Addr);
> > > +              out.d_type = in.d_type = ELF_T_ADDR;
> > > +              if (elf64_xlatetom (&out, &in, elf_getident (elf, NULL)[EI_DATA]) != NULL)
> > > +                  sym_addr = sym.st_value + bias;
> > 
> > You are ignoring the error case. You are reusing the original
> > sym.st_value, but haven't stored the newly fetched address in it (might
> > be better to do that in a fresh variable anyway). Note that you are
> > reusing bias above for both the dwfl_module_address_section () and
> > dwfl_module_getelf () calls, you are using the result of the second, not
> > the first here.
> 
> I have a few doubts here....basically the dwfl_module_address_section()
> uses 'sym_addr' variable and does not store data into it but we are
> using it afresh after initialisation. I'm thinking if the 'sym_addr' in
> the pseudocode referred to some other value which I did not decipher
> correctly.

dwfl_module_address_section () will adjust the given sym_addr to be
relative to the section (see the description in libdwfl.h). sym_addr was
originally initialized to sym.st_value, but I now see you changed that
in your patch.

> Secondly, I see that 'bias' will result in the same value from both
> function calls - dwfl_module_address_section and dwfl_module_getelf
> given that the same module 'm' is passed to them, so guess we don't have
> to be worried at that.

The bias from dwfl_module_address_section () is set to "the difference
between addresses used in the returned section's headers and run-time
addresses". The bias from dwfl_module_getelf () is set to "the
difference between addresses within the loaded module and those in
symbol tables or Dwarf information referring to it". I always get
confused by these biases, so I am normally conservative in making sure I
don't mix them if I am not 100% convinced they will always be the same.
I would have to look at the source to be truly sure here.

> What isn't clear to me is the error case you've mentioned and the new
> address you'd like to store in sym.st_value.

You have to handle errors, either by dropping out and producing an error
or ignoring them/warning about them. I am not suggesting you store
anything new in sym.st_value. But in the end sym_addr needs to point to
the actual function address, since that is what we will use to store the
address of the function for the section in the address map (at the end
of the for loop as (addrmap[secidx])[sym_addr] = name). This then gets
put in the stap-symbols.h table.

BTW. David Smith found another instance of what you are working on, see
http://sourceware.org/bugzilla/show_bug.cgi?id=12566
"usymbols.exp 64-bit test failing on ppc64"
You might want to post your current incarnation of your patch so you can
coordinate with him to see if it helps his testcase.

Cheers,

Mark