Re: WIP fix for PR 12997

[Mark Wielaard <mjw at redhat dot com>]

On Mon, Mar 19, 2012 at 09:05:42AM -0600, Tom Tromey wrote:
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> How about this one instead?  It includes a test case.  The test case
> fails with an unmodified stap, and passes with the rest of the patch
> applied.

I like it. Thanks. Some comments below.

> diff --git a/dwflpp.cxx b/dwflpp.cxx
> index 7a659c4..17f3bf8 100644
> --- a/dwflpp.cxx
> +++ b/dwflpp.cxx
>  void
>  dwflpp::iterate_over_cus (int (*callback)(Dwarf_Die * die, void * arg),
> -                          void * data)
> +                          void * data, bool want_types)
>  {
>    get_module_dwarf(false);
>    Dwarf *dw = module_dwarf;
> @@ -431,6 +433,26 @@ dwflpp::iterate_over_cus (int (*callback)(Dwarf_Die * die, void * arg),
>          }
>      }
>  
> +  if (want_types && !this->loaded_type_units)
> +    {
> +      // Process type units.
> +      Dwarf_Off off = 0;
> +      size_t cuhl;
> +      Dwarf_Off noff;
> +      uint64_t type_signature;
> +      while (dwarf_next_unit (dw, off, &noff, &cuhl, NULL, NULL, NULL, NULL,
> +			      &type_signature, NULL) == 0)
> +	{
> +          if (pending_interrupts) return;
> +          Dwarf_Die die_mem;
> +          Dwarf_Die *die;
> +          die = dwarf_offdie_types (dw, off + cuhl, &die_mem);
> +          v->push_back (*die); /* copy */
> +          off = noff;
> +	}
> +      this->loaded_type_units = true;
> +    }
> +

I like this solution of using an explicit want_types.
But I am confused about the this->loaded_type_units thing.
That seems in the wrong place. Which caller needs this?
With this the first caller to iterate_over_cus() that sets want_types,
will get its callback called for all DIEs, in both CUs and TUs, but
any next one will get its callback called only for all DIEs in the CUs.
That is somewhat confusing, at least to me.

> diff --git a/testsuite/systemtap.pass1-4/debugtypes.cxx b/testsuite/systemtap.pass1-4/debugtypes.cxx
> new file mode 100644
> index 0000000..faaebab
> --- /dev/null
> +++ b/testsuite/systemtap.pass1-4/debugtypes.cxx
> @@ -0,0 +1,21 @@
> +struct s1
> +{
> +  char c;
> +  short s;
> +  int i;
> +  long l;
> +  float f;
> +  double d;
> +};
> +
> +s1 S1;
> +
> +int func (s1 *p)
> +{
> +  return p->i;
> +}
> +
> +int main()
> +{
> +  return func (&S1);
> +}
> diff --git a/testsuite/systemtap.pass1-4/debugtypes.stp b/testsuite/systemtap.pass1-4/debugtypes.stp
> new file mode 100755
> index 0000000..4a96d1d
> --- /dev/null
> +++ b/testsuite/systemtap.pass1-4/debugtypes.stp
> @@ -0,0 +1,5 @@
> +#! stap -p2
> +
> +probe process("debugtypes.exe").function("func") {
> +  println(@cast($p, "struct s1")->l)
> +}

You shouldn't need @cast here.
Or if that is deliberate a better test would be to do it twice
and see if it gets the same result:

println($p->l);
println(@cast($p, "struct s1")->l);

Thanks,

Mark