[RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

Hi all,
This patch modifies tapsets.cxx to enable members of anonymous structs/ 
unions to be recognised by the systemtap translator.
Pls let me know your comments..

Regards,
Prerna Saxena

[-- Attachment #2: anon-union-1.patch --]
[-- Type: text/plain, Size: 3040 bytes --]

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1866,6 +1866,8 @@ struct dwflpp
   {
     Dwarf_Die *die = vardie;
     Dwarf_Die struct_die;
+    Dwarf_Attribute temp_attr;
+
     unsigned i = 0;
     while (i < components.size())
       {
@@ -1924,9 +1926,9 @@ struct dwflpp
 	    switch (dwarf_child (die, die_mem))
 	      {
 	      case 1:		/* No children.  */
-		throw semantic_error ("empty struct "
-				      + string (dwarf_diename_integrate (die) ?: "<anonymous>"));
-		break;
+		clog<<"\n Empty Struct "
+		    <<(dwarf_diename_integrate(die)?:"<anonymous>");
+		return NULL;
 	      case -1:		/* Error.  */
 	      default:		/* Shouldn't happen */
 		throw semantic_error (string (typetag == DW_TAG_union_type ? "union" : "struct")
@@ -1941,14 +1943,36 @@ struct dwflpp
 	    while (dwarf_tag (die) != DW_TAG_member
 		   || ({ const char *member = dwarf_diename_integrate (die);
 		       member == NULL || string(member) != components[i].second; }))
+	    {
+	      if ( dwarf_diename (die) == NULL )  // handling Anonymous structs/unions
+		{ 
+		   Dwarf_Die temp_die = *die;
+		   
+		   if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                      	{ 
+			  clog<<"\n Error in obtaining type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                	  return NULL;
+                	}
+
+	           if ( !dwarf_formref_die (&temp_attr,&temp_die) )
+        	        { 
+			  clog<<"\n Error in decoding type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                	  return NULL;
+                	}
+
+                   Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die, &temp_attr );
+
+		   if (result_die != NULL)
+			{ 
+			  *attr_mem = temp_attr;
+			  return result_die;
+			}
+		}
 	      if (dwarf_siblingof (die, die_mem) != 0)
-	      {
-		  stringstream alternatives;
-		  print_members (&struct_die, alternatives);
-		  throw semantic_error ("field '" + components[i].second
-					+ "' not found (alternatives:"
-					+ alternatives.str () + ")");
-	      }
+		return NULL;
+	    }
 
 	    if (dwarf_attr_integrate (die, DW_AT_data_member_location,
 				      attr_mem) == NULL)
@@ -2230,6 +2254,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation failure");
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2297,6 +2323,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation Failure");
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Frank Ch. Eigler]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

> This patch modifies tapsets.cxx to enable members of anonymous
> structs/ unions to be recognised by the systemtap translator.
> Pls let me know your comments..

It should be a nice improvement.  The code needs some test cases, and
perhaps you could transcribe here "before & after" messages that could
be summarized also into src/NEWS.

The code that prints to clog needs to go back to throwing exceptions,
or if absolutely necessary, to session.print_{warning,error}.


- FChE

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Hi Prerna,

Prerna Saxena wrote:
> Hi all,
> This patch modifies tapsets.cxx to enable members of anonymous structs/ 
> unions to be recognised by the systemtap translator.
> Pls let me know your comments..

I'm interested in this feature.

I tested your patches on x86-64 with elfutils-0.137, and I got a SEGV
when I ran following command:

$ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain") 
{print($port_dev->ex_dev->children)}' -vvvp2
SystemTap translator/driver (version 0.7.1/0.137 git branch master, 
commit 38fc2504 + changes)
Copyright (C) 2005-2008 Red Hat, Inc. and others
This is free software; see the source for copying conditions.
Session arch: x86_64 release: 2.6.18-92.el5
Created temporary directory "/tmp/stapQefK9v"
Searched '/usr/share/systemtap/tapset/x86_64/*.stp', found 2
Searched '/usr/share/systemtap/tapset/*.stp', found 41
Pass 1: parsed user script and 43 library script(s) in 
380usr/20sys/533real ms.
control symbols: kts: 0xffffffff80064c68 kte: 0xffffffff80067956 stext: 
0xffffffff80001000
parsed 'sas_ex_revalidate_domain' -> func 'sas_ex_revalidate_domain'
blacklist regexps:
blfn: 
^(atomic_notifier_call_chain|default_do_nmi|__die|die_nmi|do_debug|do_general_protection|do_int3|do_IRQ|do_page_fault|do_sparc64_fault|do_trap|dummy_nmi_callback|flush_icache_range|ia64_bad_break|ia64_do_page_fault|ia64_fault|io_check_error|mem_parity_error|nmi_watchdog_tick|notifier_call_chain|oops_begin|oops_end|program_check_exception|single_step_exception|sync_regs|unhandled_fault|unknown_nmi_error|.*raw_.*lock.*|.*read_.*lock.*|.*write_.*lock.*|.*spin_.*lock.*|.*rwlock_.*lock.*|.*rwsem_.*lock.*|.*mutex_.*lock.*|raw_.*|.*seq_.*lock.*|atomic_.*|atomic64_.*|get_bh|put_bh|.*apic.*|.*APIC.*|.*softirq.*|.*IRQ.*|.*_intr.*|__delay|.*kernel_text.*|get_current|current_.*|.*exception_tables.*|.*setup_rt_frame.*|.*preempt_count.*|preempt_schedule|__switch_to)$
blfn_ret: ^(do_exit|sys_exit|sys_exit_group)$
blfile: ^(kernel/kprobes.c|arch/.*/kernel/kprobes.c)$
focused on module 'libsas = [0x759a600-0x75aadc9, bias 0x0] file 
/usr/lib/debug/lib/modules/2.6.18-92.el5/kernel/drivers/scsi/libsas/libsas.ko.debug 
ELF machine x86_64 (code 62)
selected function sas_ex_revalidate_domain
probe sas_ex_revalidate_domain@drivers/scsi/libsas/sas_expander.c:1861 
module=libsas reloc=.text section=.text pc=0x759ded1
finding location for local 'port_dev' near address 759ded1, module bias 0
Segmentation fault

----
And here is the output of gdb

$ gdb stap
GNU gdb Red Hat Linux (6.5-37.el5rh)
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain 
conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu"...Using host 
libthread_db library "/lib64/libthread_db.so.1".

(gdb) r -e 'probe module("libsas").function("sas_ex_revalidate_domain") 
{print($port_dev->ex_dev->children)}'
Starting program: /usr/bin/stap -e 'probe 
module("libsas").function("sas_ex_revalidate_domain") 
{print($port_dev->ex_dev->children)}'
[Thread debugging using libthread_db enabled]
[New Thread 47364313547600 (LWP 3183)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 47364313547600 (LWP 3183)]
0x00002b13dbba7fd3 in __libdw_find_attr (die=0x7fffcf43f930, search_name=3,
     codep=0x7fffcf43f9c0, formp=0x7fffcf43f9c4)
     at elfutils-0.137/libdw/dwarf_child.c:67
67	elfutils-0.137/libdw/dwarf_child.c: No such file or directory.
	in elfutils-0.137/libdw/dwarf_child.c
Current language:  auto; currently c
(gdb) p *die
$1 = {addr = 0x2b13e69c2d6b, cu = 0x765c800000aa3d00, abbrev = 
0x7fffcf43f930,
   padding__ = 140736670726592}

-----
it seems that die is not initialized.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Masami Hiramatsu wrote:
> Hi Prerna,
> 
> Prerna Saxena wrote:
>> Hi all,
>> This patch modifies tapsets.cxx to enable members of anonymous structs/ 
>> unions to be recognised by the systemtap translator.
>> Pls let me know your comments..
> 
> I'm interested in this feature.
> 
> I tested your patches on x86-64 with elfutils-0.137, and I got a SEGV
> when I ran following command:
> 
> $ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain") 
> {print($port_dev->ex_dev->children)}' -vvvp2

Curiously, on i386, it just shows an error.

$ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain")
{print($port_dev->ex_dev->children)}' -vp2
Pass 1: parsed user script and 45 library script(s) in 300usr/0sys/307real ms.
semantic error: struct/union 'children' is being accessed instead of a member of the struct/union: identifier '$port_dev' at <input>:2:8
Pass 2: analyzed script: 1 probe(s), 0 function(s), 0 embed(s), 0 global(s) in 90usr/310sys/398real ms.
Pass 2: analysis failed.  Try again with more '-v' (verbose) options.

Actually, previous example is not correct script. :-(

And following a 'correct' script works on i386/x86-64 with your patch.

$ stap -ve 'probe kernel.function("block_sync_page") {print($page->private)}' -p4
Pass 1: parsed user script and 45 library script(s) in 310usr/0sys/315real ms.
Pass 2: analyzed script: 1 probe(s), 1 function(s), 0 embed(s), 0 global(s) in 300usr/320sys/620real ms.
Pass 3: translated to C into "/tmp/stapEgPsRp/stap_0299bd2b710f6d0969906b7d8f2a6301_734.c" in 180usr/360sys/543real ms.
/home/mhiramat/.systemtap/cache/02/stap_0299bd2b710f6d0969906b7d8f2a6301_734.ko
Pass 4: compiled C into "stap_0299bd2b710f6d0969906b7d8f2a6301_734.ko" in 3080usr/400sys/3526real ms.


So, I think your patch correctly works on i386, but there is
a bug on x86-64.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

Frank Ch. Eigler wrote:
> Prerna Saxena <prerna@linux.vnet.ibm.com> writes:
>
>   
>> This patch modifies tapsets.cxx to enable members of anonymous
>> structs/ unions to be recognised by the systemtap translator.
>> Pls let me know your comments..
>>     
>
> It should be a nice improvement.  The code needs some test cases, and
> perhaps you could transcribe here "before & after" messages that could
> be summarized also into src/NEWS.
>
> The code that prints to clog needs to go back to throwing exceptions,
> or if absolutely necessary, to session.print_{warning,error}.
>
>
> - FChE
>   

Hi Frank,
I had deliberately changed code in function translate_components() to  
print  to clog instead of throwing exceptions.
 Initially  this function was called only once wherein it would search 
for the dwarf information for a member of a given parent struct / union 
; or flag an error if the member wasnt found or errors were encountered. 
I have modified the function to be called recursively for each anonymous 
structs/ unions encountered in the parent struct/ union.

I thought it would not be a good idea to make the code throw an 
exception if a there are issues determining the dwarf information for 
one search path traversing one of the nested structs ; there is a 
reasonably good chance this member may be discovered in another search 
path and the translation may succeed later on.
Pls let me know your comments..

Thanks & regards,
Prerna

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Frank Ch. Eigler]

Prerna Saxena <prerna@linux.vnet.ibm.com> writes:

>> [...]
>> The code that prints to clog needs to go back to throwing exceptions,
>> or if absolutely necessary, to session.print_{warning,error}.

> I had deliberately changed code in function translate_components() to
> print  to clog instead of throwing exceptions.

Right.

> [...]  I thought it would not be a good idea to make the code throw
> an exception if a there are issues determining the dwarf information
> for one search path traversing one of the nested structs ; there is
> a reasonably good chance this member may be discovered in another
> search path and the translation may succeed later on.  [...]

The problem with this line of thought is that the user will see
messages that he won't know what to do with.  There should be no
message on clog at all if the search succeeds down another branch.

One can use exceptions in this context just fine.  When you recurse,
you can catch the exceptions, only to throw a new (or a saved old) one
should the final search results come up empty.


- FChE

Re: [RFC PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Masami Hiramatsu wrote:
> Masami Hiramatsu wrote:
>> Hi Prerna,
>>
>> Prerna Saxena wrote:
>>> Hi all,
>>> This patch modifies tapsets.cxx to enable members of anonymous structs/ 
>>> unions to be recognised by the systemtap translator.
>>> Pls let me know your comments..
>> I'm interested in this feature.
>>
>> I tested your patches on x86-64 with elfutils-0.137, and I got a SEGV
>> when I ran following command:
>>
>> $ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain") 
>> {print($port_dev->ex_dev->children)}' -vvvp2

Finally, I got a clue, here is a part of gdb log.

----
Starting program: /usr/bin/stap -e 'probe module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' -vp2
Breakpoint 1 at 0x2ac797678930: file tapsets.cxx, line 2164.
Breakpoint 2 at 0x2ac79766e640: file tapsets.cxx, line 2002.
[Thread debugging using libthread_db enabled]
Pass 1: parsed user script and 45 library script(s) in 260usr/20sys/275real ms.
[New Thread 47036754412368 (LWP 4726)]
[Switching to Thread 47036754412368 (LWP 4726)]

Breakpoint 1, dwflpp::literal_stmt_for_local (this=0x2ac79a09c3d0,
    scope_die=0x2ac7a2757778, pc=123498677, local=@0x7fff13502350,
    components=@0x2ac7a2757c68, lvalue=false, ty=@0x2ac7a3062198)
    at tapsets.cxx:2164
2164				  exp_type & ty)
(gdb) c
Continuing.

Breakpoint 2, dwflpp::translate_final_fetch_or_store (this=0x2ac79a09c3d0,
    pool=0x7fff13501c90, tail=0x7fff13501d98, module_bias=0,
    die=0x7fff135017f0, attr_mem=0x7fff13501d30, lvalue=false,
    ty=@0x2ac7a3062198) at tapsets.cxx:2002
2002					  exp_type & ty)
(gdb) p *die
$13 = {addr = 0x2ac7a2f13adf, cu = 0x2ac7a2745e00, abbrev = 0x2ac7a2751088,
  padding__ = 0}
(gdb) p *die->cu
$14 = {dbg = 0x2ac7a2745ae0, start = 312246, end = 380691,
  address_size = 8 '\b', offset_size = 4 '\004', version = 2, abbrev_hash = {
    size = 167, filled = 120, table = 0x2ac7a2753580},
  orig_abbrev_offset = 6545, last_abbrev_offset = 8087,
  lines = 0x2ac7a3048028, files = 0x2ac7a27568a8, locs = 0x2ac7a3062200}
-----
Actually, this die->cu was broken when stap got a SEGV.
It's address(die->cu) is "0x7fff135017f0" + 8
-----
(gdb) watch die->cu
Watchpoint 3: die->cu
(gdb) c
Continuing.
Watchpoint 3: die->cu

Old value = (struct Dwarf_CU *) 0x2ac7a2745e00
New value = (struct Dwarf_CU *) 0x5ea1e000001f4600
0x00002ac797aecb9a in __libdw_formref (attr=0x7fff13501d30,
    return_offset=0x7fff13501818) at elfutils-0.137/libdw/dwarf_formref.c:62
62	elfutils-0.137/libdw/dwarf_formref.c: No such file or directory.
	in elfutils-0.137/libdw/dwarf_formref.c
Current language:  auto; currently c
----
Here, die->cu was overwritten.
----
(gdb) l
57	in elfutils-0.137/libdw/dwarf_formref.c

(gdb) disassemble
Dump of assembler code for function __libdw_formref:
0x00002ac797aecb80 <__libdw_formref+0>:	push   %rbx
0x00002ac797aecb81 <__libdw_formref+1>:	mov    %rsi,%rbx
0x00002ac797aecb84 <__libdw_formref+4>:	sub    $0x10,%rsp
0x00002ac797aecb88 <__libdw_formref+8>:	mov    0x8(%rdi),%rcx
0x00002ac797aecb8c <__libdw_formref+12>:	mov    %fs:0x28,%rax
0x00002ac797aecb95 <__libdw_formref+21>:	mov    %rax,0x8(%rsp)
0x00002ac797aecb9a <__libdw_formref+26>:	xor    %eax,%eax
-----
Curiously, the assembler code just push a value to stack,
-----
(gdb) p $rsp
$23 = (void *) 0x7fff135017f0
-----

You can see the current stack pointer is same as the address of 'die'.
This means, the 'die' originally has been stored on the stack
memory (as a local variable) and passed it back to caller, and caller
reuse this stack.

I think below code is a suspicious code.

> +                   Dwarf_Die *result_die = translate_components(pool, tail,
> pc, components, &temp_die, &temp_die, &temp_attr );

Since temp_die is just a local variable, I think secound &temp_die(6th argument)
should be die_mem as same as original function.


Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC updated PATCH 2/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]

Hi Frank,
As per your suggestion , I've removed the clog messages and instead 
replaced them with  a global (static) string to contain error messages 
in case of errors encountered in reading/decoding dwarf information. 
This brings 2 scenarios :

i  . An error is encountered in a search path but the listed member is 
found in another search path :
    No error message displayed in this case.

ii . An error is encountered in reading dwarf info for a search path AND 
the listed member is not found in any search path :
    The respective error msg is listed at the end when a semantic error 
is thrown for Translation failure.

This seems to work okay for now, but I'm looking forward to suggestions 
for improving it :-)

Frank Ch. Eigler wrote:
> Prerna Saxena <prerna@linux.vnet.ibm.com> writes:
>
>   
>>> [...]
>>> The code that prints to clog needs to go back to throwing exceptions,
>>> or if absolutely necessary, to session.print_{warning,error}.
>>>       
>
>   
>> I had deliberately changed code in function translate_components() to
>> print  to clog instead of throwing exceptions.
>>     
>
> Right.
>
>   
>> [...]  I thought it would not be a good idea to make the code throw
>> an exception if a there are issues determining the dwarf information
>> for one search path traversing one of the nested structs ; there is
>> a reasonably good chance this member may be discovered in another
>> search path and the translation may succeed later on.  [...]
>>     
>
> The problem with this line of thought is that the user will see
> messages that he won't know what to do with.  There should be no
> message on clog at all if the search succeeds down another branch.
>
> One can use exceptions in this context just fine.  When you recurse,
> you can catch the exceptions, only to throw a new (or a saved old) one
> should the final search results come up empty.
>
>
> - FChE
>   
Regards,

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: anon-union-2.patch --]
[-- Type: text/plain, Size: 2391 bytes --]

Signed-off-by : Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1846,9 +1846,33 @@ struct dwflpp
     // Output each sibling's name to 'o'.
     while (dwarf_tag (die) == DW_TAG_member)
       {
-	const char *member = (dwarf_diename_integrate (die) ?: "<anonymous>");
+	const char *member = dwarf_diename_integrate (die) ;
+	
+	if ( member != NULL )
+
+		o << " " << member;
+
+	else
+	    { 
+		Dwarf_Die temp_die = *die; 
+		Dwarf_Attribute temp_attr ;
+		
+		 if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                        {
+                          clog<<"\n Error in obtaining type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+
+                   if ( ! dwarf_formref_die (&temp_attr,&temp_die))
+                        {
+                          clog<<"\n Error in decoding type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+		   print_members(&temp_die,o);
 
-	o << " " << member;
+	    }
 
 	if (dwarf_siblingof (die, &die_mem) != 0)
 	  break;
@@ -2256,7 +2280,12 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, &vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure : " + error_msg + "\n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2326,7 +2355,13 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation Failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure : " + error_msg + "\n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
+
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC updated PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 4379 bytes --]

Hi all,
I'm posting patch 1 of 2 after fixing a bug which was causing it to fail 
on x86_64.

@Masami,  thanks for pointing me to it :-)

Masami Hiramatsu wrote:
> Masami Hiramatsu wrote:
>   
>> Masami Hiramatsu wrote:
>>     
>>> Hi Prerna,
>>>
>>> Prerna Saxena wrote:
>>>       
>>>> Hi all,
>>>> This patch modifies tapsets.cxx to enable members of anonymous structs/ 
>>>> unions to be recognised by the systemtap translator.
>>>> Pls let me know your comments..
>>>>         
>>> I'm interested in this feature.
>>>
>>> I tested your patches on x86-64 with elfutils-0.137, and I got a SEGV
>>> when I ran following command:
>>>
>>> $ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain") 
>>> {print($port_dev->ex_dev->children)}' -vvvp2
>>>       
>
> Finally, I got a clue, here is a part of gdb log.
>
> ----
> Starting program: /usr/bin/stap -e 'probe module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' -vp2
> Breakpoint 1 at 0x2ac797678930: file tapsets.cxx, line 2164.
> Breakpoint 2 at 0x2ac79766e640: file tapsets.cxx, line 2002.
> [Thread debugging using libthread_db enabled]
> Pass 1: parsed user script and 45 library script(s) in 260usr/20sys/275real ms.
> [New Thread 47036754412368 (LWP 4726)]
> [Switching to Thread 47036754412368 (LWP 4726)]
>
> Breakpoint 1, dwflpp::literal_stmt_for_local (this=0x2ac79a09c3d0,
>     scope_die=0x2ac7a2757778, pc=123498677, local=@0x7fff13502350,
>     components=@0x2ac7a2757c68, lvalue=false, ty=@0x2ac7a3062198)
>     at tapsets.cxx:2164
> 2164				  exp_type & ty)
> (gdb) c
> Continuing.
>
> Breakpoint 2, dwflpp::translate_final_fetch_or_store (this=0x2ac79a09c3d0,
>     pool=0x7fff13501c90, tail=0x7fff13501d98, module_bias=0,
>     die=0x7fff135017f0, attr_mem=0x7fff13501d30, lvalue=false,
>     ty=@0x2ac7a3062198) at tapsets.cxx:2002
> 2002					  exp_type & ty)
> (gdb) p *die
> $13 = {addr = 0x2ac7a2f13adf, cu = 0x2ac7a2745e00, abbrev = 0x2ac7a2751088,
>   padding__ = 0}
> (gdb) p *die->cu
> $14 = {dbg = 0x2ac7a2745ae0, start = 312246, end = 380691,
>   address_size = 8 '\b', offset_size = 4 '\004', version = 2, abbrev_hash = {
>     size = 167, filled = 120, table = 0x2ac7a2753580},
>   orig_abbrev_offset = 6545, last_abbrev_offset = 8087,
>   lines = 0x2ac7a3048028, files = 0x2ac7a27568a8, locs = 0x2ac7a3062200}
> -----
> Actually, this die->cu was broken when stap got a SEGV.
> It's address(die->cu) is "0x7fff135017f0" + 8
> -----
> (gdb) watch die->cu
> Watchpoint 3: die->cu
> (gdb) c
> Continuing.
> Watchpoint 3: die->cu
>
> Old value = (struct Dwarf_CU *) 0x2ac7a2745e00
> New value = (struct Dwarf_CU *) 0x5ea1e000001f4600
> 0x00002ac797aecb9a in __libdw_formref (attr=0x7fff13501d30,
>     return_offset=0x7fff13501818) at elfutils-0.137/libdw/dwarf_formref.c:62
> 62	elfutils-0.137/libdw/dwarf_formref.c: No such file or directory.
> 	in elfutils-0.137/libdw/dwarf_formref.c
> Current language:  auto; currently c
> ----
> Here, die->cu was overwritten.
> ----
> (gdb) l
> 57	in elfutils-0.137/libdw/dwarf_formref.c
>
> (gdb) disassemble
> Dump of assembler code for function __libdw_formref:
> 0x00002ac797aecb80 <__libdw_formref+0>:	push   %rbx
> 0x00002ac797aecb81 <__libdw_formref+1>:	mov    %rsi,%rbx
> 0x00002ac797aecb84 <__libdw_formref+4>:	sub    $0x10,%rsp
> 0x00002ac797aecb88 <__libdw_formref+8>:	mov    0x8(%rdi),%rcx
> 0x00002ac797aecb8c <__libdw_formref+12>:	mov    %fs:0x28,%rax
> 0x00002ac797aecb95 <__libdw_formref+21>:	mov    %rax,0x8(%rsp)
> 0x00002ac797aecb9a <__libdw_formref+26>:	xor    %eax,%eax
> -----
> Curiously, the assembler code just push a value to stack,
> -----
> (gdb) p $rsp
> $23 = (void *) 0x7fff135017f0
> -----
>
> You can see the current stack pointer is same as the address of 'die'.
> This means, the 'die' originally has been stored on the stack
> memory (as a local variable) and passed it back to caller, and caller
> reuse this stack.
>
> I think below code is a suspicious code.
>
>   
>> +                   Dwarf_Die *result_die = translate_components(pool, tail,
>> pc, components, &temp_die, &temp_die, &temp_attr );
>>     
>
> Since temp_die is just a local variable, I think secound &temp_die(6th argument)
> should be die_mem as same as original function.
>
>
> Thank you,
>
>   

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: anon-union-1.patch --]
[-- Type: text/plain, Size: 3555 bytes --]

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -64,6 +64,7 @@ extern "C" {
 using namespace std;
 using namespace __gnu_cxx;
 
+static string error_msg("");
 // ------------------------------------------------------------------------
 // Generic derived_probe_group: contains an ordinary vector of the
 // given type.  It provides only the enrollment function.
@@ -1866,6 +1867,8 @@ struct dwflpp
   {
     Dwarf_Die *die = vardie;
     Dwarf_Die struct_die;
+    Dwarf_Attribute temp_attr;
+
     unsigned i = 0;
     while (i < components.size())
       {
@@ -1924,9 +1927,9 @@ struct dwflpp
 	    switch (dwarf_child (die, die_mem))
 	      {
 	      case 1:		/* No children.  */
-		throw semantic_error ("empty struct "
-				      + string (dwarf_diename_integrate (die) ?: "<anonymous>"));
-		break;
+		clog<<"\n Empty Struct "
+		    <<(dwarf_diename_integrate(die)?:"<anonymous>");
+		return NULL;
 	      case -1:		/* Error.  */
 	      default:		/* Shouldn't happen */
 		throw semantic_error (string (typetag == DW_TAG_union_type ? "union" : "struct")
@@ -1941,14 +1944,35 @@ struct dwflpp
 	    while (dwarf_tag (die) != DW_TAG_member
 		   || ({ const char *member = dwarf_diename_integrate (die);
 		       member == NULL || string(member) != components[i].second; }))
+	    {
+	      if ( dwarf_diename (die) == NULL )  // handling Anonymous structs/unions
+		{ 
+		   Dwarf_Die temp_die = *die;
+		   Dwarf_Die temp_die_2;
+		   
+		   if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                      	{ 
+			  error_msg = "\n Error in obtaining type attribute for "+ string(dwarf_diename(&temp_die)?:"<anonymous>");
+                	  return NULL;
+                	}
+
+	           if ( !dwarf_formref_die (&temp_attr,&temp_die) )
+        	        { 
+			  error_msg = "\n Error in decoding DW_AT_type attribute for " + string(dwarf_diename(&temp_die)?:"<anonymous>");
+                	  return NULL;
+                	}
+
+                   Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die_2, &temp_attr );
+
+		   if (result_die != NULL)
+			{ 
+			  *attr_mem = temp_attr;
+			  return result_die;
+			}
+		}
 	      if (dwarf_siblingof (die, die_mem) != 0)
-	      {
-		  stringstream alternatives;
-		  print_members (&struct_die, alternatives);
-		  throw semantic_error ("field '" + components[i].second
-					+ "' not found (alternatives:"
-					+ alternatives.str () + ")");
-	      }
+		return NULL;
+	    }
 
 	    if (dwarf_attr_integrate (die, DW_AT_data_member_location,
 				      attr_mem) == NULL)
@@ -2228,8 +2252,11 @@ struct dwflpp
     /* Translate the ->bar->baz[NN] parts. */
 
     Dwarf_Die die_mem, *die = NULL;
+    error_msg = "";
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation failure");
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2295,8 +2322,11 @@ struct dwflpp
     Dwarf_Die *vardie = dwarf_formref_die (attr, &vardie_mem);
 
     Dwarf_Die die_mem, *die = NULL;
+    error_msg = "";
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation Failure");
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC updated PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Hi Prerna,

Prerna Saxena wrote:
> Hi all,
> I'm posting patch 1 of 2 after fixing a bug which was causing it to fail 
> on x86_64.
> 
> @Masami,  thanks for pointing me to it :-)
> 
> Masami Hiramatsu wrote:
>> I think below code is a suspicious code.
>>
>>   
>>> +                   Dwarf_Die *result_die = translate_components(pool, tail,
>>> pc, components, &temp_die, &temp_die, &temp_attr );
>>>     
>> Since temp_die is just a local variable, I think secound &temp_die(6th argument)
>> should be die_mem as same as original function.

As far as I can see, your patch still use a local variable for returning
memory buffer(temp_die_2).

> +		   Dwarf_Die temp_die_2;
[snip]
> +                   Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die_2, &temp_attr );
> +

If translate_components found a matched die, it stores that die information into
die_mem(temp_die_2, in your code) and returns the address of die_mem(&temp_die_2)
to caller.

> +		   if (result_die != NULL)
> +			{ 
> +			  *attr_mem = temp_attr;
> +			  return result_die;
> +			}

In your code, if result_die (= &temp_die_2) is not NULL, returns it to
caller again. This means, translate_components returns it's local memory
on stack which will be used by other functions in the future.
(Note: Without your patch, translate_components returns vardie or die_mem,
both of which are passed from caller and not on local memory.)

So, I think you must use die_mem which is passed by other functions(
literal_stmt_for_local or literal_stmt_for_return) instead of &temp_die_2.

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC updated PATCH 2/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Frank Ch. Eigler]

Hi -

> As per your suggestion , I've removed the clog messages and instead 
> replaced them with  a global (static) string to contain error messages 
> in case of errors encountered in reading/decoding dwarf information. 

(Actually, there remain one or two hard-coded clog references in the
patch, unless I'm misreading.)

The use of the static error_msg string is unnecessary.  I advised
having an explicit try {} catch () within the search iterations, so
that each layer can just throw an exception if something is wrong.
The parent can catch exceptions and try another branch.  If the search
is unsuccessful, it can throw a new (or saved/reused) exception of its
own.  This should also simplify the logic.

- FChE

Re: [RFC updated PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 2751 bytes --]

Hi Masami,

Masami Hiramatsu wrote:
> Hi Prerna,
>
> Prerna Saxena wrote:
>   
>> Hi all,
>> I'm posting patch 1 of 2 after fixing a bug which was causing it to fail 
>> on x86_64.
>>
>> @Masami,  thanks for pointing me to it :-)
>>
>> Masami Hiramatsu wrote:
>>     
>>> I think below code is a suspicious code.
>>>
>>>   
>>>       
>>>> +                   Dwarf_Die *result_die = translate_components(pool, tail,
>>>> pc, components, &temp_die, &temp_die, &temp_attr );
>>>>     
>>>>         
>>> Since temp_die is just a local variable, I think secound &temp_die(6th argument)
>>> should be die_mem as same as original function.
>>>       
>
> As far as I can see, your patch still use a local variable for returning
> memory buffer(temp_die_2).
>
>   
>> +		   Dwarf_Die temp_die_2;
>>     
> [snip]
>   
>> +                   Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die_2, &temp_attr );
>> +
>>     
>
> If translate_components found a matched die, it stores that die information into
> die_mem(temp_die_2, in your code) and returns the address of die_mem(&temp_die_2)
> to caller.
>
>   
>> +		   if (result_die != NULL)
>> +			{ 
>> +			  *attr_mem = temp_attr;
>> +			  return result_die;
>> +			}
>>     
>
> In your code, if result_die (= &temp_die_2) is not NULL, returns it to
> caller again. This means, translate_components returns it's local memory
> on stack which will be used by other functions in the future.
> (Note: Without your patch, translate_components returns vardie or die_mem,
> both of which are passed from caller and not on local memory.)
>
> So, I think you must use die_mem which is passed by other functions(
> literal_stmt_for_local or literal_stmt_for_return) instead of &temp_die_2.
>
> Thank you,
>
>   
Agree, but making "die_mem" as the 6th arg will disturb the call flow.
This is because in each recursive call, the 6th arg is overwritten by a 
die which is pointed to by DW_AT_type attribute of the original 
attr_mem. If die_mem is re-used as 6th arg instead of a new variable, 
the old contents of die (in the parent recursive call) will also be lost 
as both die & die_mem point to the same location, (whose contents would 
be overwritten). This is not a problem if search in a branch  has 
succeeded-- but in case a search path fails and a new branch needs to be 
tried, this will deem it impossible.

I agree with your concern about "temp_die_2" being local memory on the 
stack which may be reused, so I've fixed it by copying the contents of 
"temp_die_2" to "die_mem" in case of a successful match. This should 
take care of memory errors.

Regards,

-- 

Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: anon-union-1.patch --]
[-- Type: text/plain, Size: 3979 bytes --]

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1866,7 +1866,15 @@ struct dwflpp
   {
     Dwarf_Die *die = vardie;
     Dwarf_Die struct_die;
+    Dwarf_Attribute temp_attr;
+
     unsigned i = 0;
+
+    static unsigned int func_call_level ;
+    static unsigned int dwarf_error_flag ; // indicates current error is dwarf error
+    static unsigned int dwarf_error_count ; // keeps track of no of dwarf errors
+    static semantic_error saved_dwarf_error("");
+
     while (i < components.size())
       {
         /* XXX: This would be desirable, but we don't get the target_symbol token,
@@ -1924,9 +1932,7 @@ struct dwflpp
 	    switch (dwarf_child (die, die_mem))
 	      {
 	      case 1:		/* No children.  */
-		throw semantic_error ("empty struct "
-				      + string (dwarf_diename_integrate (die) ?: "<anonymous>"));
-		break;
+		return NULL;
 	      case -1:		/* Error.  */
 	      default:		/* Shouldn't happen */
 		throw semantic_error (string (typetag == DW_TAG_union_type ? "union" : "struct")
@@ -1941,14 +1947,60 @@ struct dwflpp
 	    while (dwarf_tag (die) != DW_TAG_member
 		   || ({ const char *member = dwarf_diename_integrate (die);
 		       member == NULL || string(member) != components[i].second; }))
+	    {
+	      if ( dwarf_diename (die) == NULL )  // handling Anonymous structs/unions
+		{ 
+		   Dwarf_Die temp_die = *die;
+		   Dwarf_Die temp_die_2;
+
+		   try 
+		    {		   
+		      if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                       { 
+			  dwarf_error_flag ++ ;
+			  dwarf_error_count ++;
+			  throw semantic_error(" Error in obtaining type attribute for "+ string(dwarf_diename(&temp_die)?:"<anonymous>"));
+                	}
+
+	              if ( !dwarf_formref_die (&temp_attr, &temp_die))
+        	        { 
+			  dwarf_error_flag ++ ;
+			  dwarf_error_count ++;
+			  throw semantic_error(" Error in decoding DW_AT_type attribute for " + string(dwarf_diename(&temp_die)?:"<anonymous>")); 
+                	}
+
+		      func_call_level ++ ;
+
+                      Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die_2, &temp_attr );
+
+		      func_call_level -- ;
+
+		      if (result_die != NULL)
+			{ 
+			  memcpy(die_mem, &temp_die_2, sizeof(Dwarf_Die));
+			  *attr_mem = temp_attr;
+			  return result_die;
+			}
+		     }
+		   catch (const semantic_error& e)
+		     { 
+			if ( !dwarf_error_flag ) //not a dwarf error
+				throw;
+			else
+				{
+				  dwarf_error_flag = 0 ;
+				  saved_dwarf_error = e ;
+				}
+		     }
+		}
 	      if (dwarf_siblingof (die, die_mem) != 0)
-	      {
-		  stringstream alternatives;
-		  print_members (&struct_die, alternatives);
-		  throw semantic_error ("field '" + components[i].second
-					+ "' not found (alternatives:"
-					+ alternatives.str () + ")");
-	      }
+		{ 
+		  if ( func_call_level == 0 && dwarf_error_count ) // this is parent call & a dwarf error has been reported in a branch somewhere
+			throw semantic_error( saved_dwarf_error );
+		  else	
+		  	 return NULL;
+		}
+	    }
 
 	    if (dwarf_attr_integrate (die, DW_AT_data_member_location,
 				      attr_mem) == NULL)
@@ -2230,6 +2282,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation failure");
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2297,6 +2351,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation Failure");
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC updated PATCH 2/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

Thanks Frank,
I've implemented it on the lines you suggested...
Dwarf errors are also flagged as semantic errors, except that they 
additionally set a flag to indicate their nature.
The catch{} block for semantic errors is also a part of the while loop. 
It re-throws all semantic errors which are NOT due to failure in 
fetching dwarf info, while saves  the last dwarf-based error.
In case no match is found in the master (parent) call of 
translate_components() ,  the saved dwarf error is thrown again.


Frank Ch. Eigler wrote:
> Hi -
>
>   
>> As per your suggestion , I've removed the clog messages and instead 
>> replaced them with  a global (static) string to contain error messages 
>> in case of errors encountered in reading/decoding dwarf information. 
>>     
>
> (Actually, there remain one or two hard-coded clog references in the
> patch, unless I'm misreading.)
>   
Thanks for pointing that out, I removed the clog message...it wasn't 
required.. :-)
> The use of the static error_msg string is unnecessary.  I advised
> having an explicit try {} catch () within the search iterations, so
> that each layer can just throw an exception if something is wrong.
> The parent can catch exceptions and try another branch.  If the search
> is unsuccessful, it can throw a new (or saved/reused) exception of its
> own.  This should also simplify the logic.
>
> - FChE
>   
Pls let me know your comments..

Regards,

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: anon-union-2.patch --]
[-- Type: text/plain, Size: 2358 bytes --]

Signed-off-by : Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1845,9 +1845,33 @@ struct dwflpp
     // Output each sibling's name to 'o'.
     while (dwarf_tag (die) == DW_TAG_member)
       {
-	const char *member = (dwarf_diename_integrate (die) ?: "<anonymous>");
+	const char *member = dwarf_diename_integrate (die) ;
+	
+	if ( member != NULL )
+
+		o << " " << member;
+
+	else
+	    { 
+		Dwarf_Die temp_die = *die; 
+		Dwarf_Attribute temp_attr ;
+		
+		 if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                        {
+                          clog<<"\n Error in obtaining type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+
+                   if ( ! dwarf_formref_die (&temp_attr,&temp_die))
+                        {
+                          clog<<"\n Error in decoding type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+		   print_members(&temp_die,o);
 
-	o << " " << member;
+	    }
 
 	if (dwarf_siblingof (die, &die_mem) != 0)
 	  break;
@@ -2283,7 +2307,12 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, &vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure :  \n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2352,7 +2381,13 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation Failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure : \n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
+
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC updated PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Hi Prerna,

Prerna Saxena wrote:
> Agree, but making "die_mem" as the 6th arg will disturb the call flow.
> This is because in each recursive call, the 6th arg is overwritten by a 
> die which is pointed to by DW_AT_type attribute of the original 
> attr_mem. If die_mem is re-used as 6th arg instead of a new variable, 
> the old contents of die (in the parent recursive call) will also be lost 
> as both die & die_mem point to the same location, (whose contents would 
> be overwritten). This is not a problem if search in a branch  has 
> succeeded-- but in case a search path fails and a new branch needs to be 
> tried, this will deem it impossible.
> 
> I agree with your concern about "temp_die_2" being local memory on the 
> stack which may be reused, so I've fixed it by copying the contents of 
> "temp_die_2" to "die_mem" in case of a successful match. This should 
> take care of memory errors.

Hmm, you seems right, but the below command still cause SEGV.

$ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' -vp2
Pass 1: parsed user script and 45 library script(s) in 370usr/30sys/404real ms.
Segmentation fault

Would you run above command for testing & debugging?

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC updated PATCHES] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Prerna Saxena]

[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]

Hi Masami,
Thanks for letting me know..
It pointed me to an incorrect pointer reference that I had 
overlooked...Its funny why this was never uncovered in all test-runs I 
had on x86 & x86_64 :-)
Here's the updated patch...with this, your test script correctly throws 
a semantic error instead of causing SEGV.

Masami Hiramatsu wrote:
> Hi Prerna,
>
> Prerna Saxena wrote:
>> Agree, but making "die_mem" as the 6th arg will disturb the call flow.
>> This is because in each recursive call, the 6th arg is overwritten by 
>> a die which is pointed to by DW_AT_type attribute of the original 
>> attr_mem. If die_mem is re-used as 6th arg instead of a new variable, 
>> the old contents of die (in the parent recursive call) will also be 
>> lost as both die & die_mem point to the same location, (whose 
>> contents would be overwritten). This is not a problem if search in a 
>> branch  has succeeded-- but in case a search path fails and a new 
>> branch needs to be tried, this will deem it impossible.
>>
>> I agree with your concern about "temp_die_2" being local memory on 
>> the stack which may be reused, so I've fixed it by copying the 
>> contents of "temp_die_2" to "die_mem" in case of a successful match. 
>> This should take care of memory errors.
>
> Hmm, you seems right, but the below command still cause SEGV.
>
> $ stap -e 'probe 
> module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' 
> -vp2
> Pass 1: parsed user script and 45 library script(s) in 
> 370usr/30sys/404real ms.
> Segmentation fault
>
> Would you run above command for testing & debugging?
>
> Thank you,
>
Thanks & regards,

-- 
Prerna Saxena

Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India 


[-- Attachment #2: anon-union-1.patch --]
[-- Type: text/plain, Size: 4008 bytes --]

Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1866,7 +1866,15 @@ struct dwflpp
   {
     Dwarf_Die *die = vardie;
     Dwarf_Die struct_die;
+    Dwarf_Attribute temp_attr;
+
     unsigned i = 0;
+
+    static unsigned int func_call_level ;
+    static unsigned int dwarf_error_flag ; // indicates current error is dwarf error
+    static unsigned int dwarf_error_count ; // keeps track of no of dwarf errors
+    static semantic_error saved_dwarf_error("");
+
     while (i < components.size())
       {
         /* XXX: This would be desirable, but we don't get the target_symbol token,
@@ -1924,9 +1932,7 @@ struct dwflpp
 	    switch (dwarf_child (die, die_mem))
 	      {
 	      case 1:		/* No children.  */
-		throw semantic_error ("empty struct "
-				      + string (dwarf_diename_integrate (die) ?: "<anonymous>"));
-		break;
+		return NULL;
 	      case -1:		/* Error.  */
 	      default:		/* Shouldn't happen */
 		throw semantic_error (string (typetag == DW_TAG_union_type ? "union" : "struct")
@@ -1941,14 +1947,60 @@ struct dwflpp
 	    while (dwarf_tag (die) != DW_TAG_member
 		   || ({ const char *member = dwarf_diename_integrate (die);
 		       member == NULL || string(member) != components[i].second; }))
+	    {
+	      if ( dwarf_diename (die) == NULL )  // handling Anonymous structs/unions
+		{ 
+		   Dwarf_Die temp_die = *die;
+		   Dwarf_Die temp_die_2;
+
+		   try 
+		    {		   
+		      if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                       { 
+			  dwarf_error_flag ++ ;
+			  dwarf_error_count ++;
+			  throw semantic_error(" Error in obtaining type attribute for "+ string(dwarf_diename(&temp_die)?:"<anonymous>"));
+                	}
+
+	              if ( !dwarf_formref_die (&temp_attr, &temp_die))
+        	        { 
+			  dwarf_error_flag ++ ;
+			  dwarf_error_count ++;
+			  throw semantic_error(" Error in decoding DW_AT_type attribute for " + string(dwarf_diename(&temp_die)?:"<anonymous>")); 
+                	}
+
+		      func_call_level ++ ;
+
+                      Dwarf_Die *result_die = translate_components(pool, tail, pc, components, &temp_die, &temp_die_2, &temp_attr );
+
+		      func_call_level -- ;
+
+		      if (result_die != NULL)
+			{ 
+			  memcpy(die_mem, &temp_die_2, sizeof(Dwarf_Die));
+			  memcpy(attr_mem, &temp_attr, sizeof(Dwarf_Attribute));
+			  return die_mem;
+			}
+		     }
+		   catch (const semantic_error& e)
+		     { 
+			if ( !dwarf_error_flag ) //not a dwarf error
+				throw;
+			else
+				{
+				  dwarf_error_flag = 0 ;
+				  saved_dwarf_error = e ;
+				}
+		     }
+		}
 	      if (dwarf_siblingof (die, die_mem) != 0)
-	      {
-		  stringstream alternatives;
-		  print_members (&struct_die, alternatives);
-		  throw semantic_error ("field '" + components[i].second
-					+ "' not found (alternatives:"
-					+ alternatives.str () + ")");
-	      }
+		{ 
+		  if ( func_call_level == 0 && dwarf_error_count ) // this is parent call & a dwarf error has been reported in a branch somewhere
+			throw semantic_error( saved_dwarf_error );
+		  else	
+		  	 return NULL;
+		}
+	    }
 
 	    if (dwarf_attr_integrate (die, DW_AT_data_member_location,
 				      attr_mem) == NULL)
@@ -2230,6 +2282,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation failure");
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2297,6 +2351,8 @@ struct dwflpp
     Dwarf_Die die_mem, *die = NULL;
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
+    if(!die)
+	throw semantic_error("Translation Failure");
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

[-- Attachment #3: anon-union-2.patch --]
[-- Type: text/plain, Size: 2358 bytes --]

Signed-off-by : Prerna Saxena <prerna@linux.vnet.ibm.com>

Index: systemtap/tapsets.cxx
===================================================================
--- systemtap.orig/tapsets.cxx
+++ systemtap/tapsets.cxx
@@ -1845,9 +1845,33 @@ struct dwflpp
     // Output each sibling's name to 'o'.
     while (dwarf_tag (die) == DW_TAG_member)
       {
-	const char *member = (dwarf_diename_integrate (die) ?: "<anonymous>");
+	const char *member = dwarf_diename_integrate (die) ;
+	
+	if ( member != NULL )
+
+		o << " " << member;
+
+	else
+	    { 
+		Dwarf_Die temp_die = *die; 
+		Dwarf_Attribute temp_attr ;
+		
+		 if (!dwarf_attr_integrate (&temp_die, DW_AT_type, &temp_attr))
+                        {
+                          clog<<"\n Error in obtaining type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+
+                   if ( ! dwarf_formref_die (&temp_attr,&temp_die))
+                        {
+                          clog<<"\n Error in decoding type attribute for "
+			      <<(dwarf_diename(&temp_die)?:"<anonymous>");
+                          return ;
+                        }
+		   print_members(&temp_die,o);
 
-	o << " " << member;
+	    }
 
 	if (dwarf_siblingof (die, &die_mem) != 0)
 	  break;
@@ -2283,7 +2307,12 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				&vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, &vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure :  \n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
 
     /* Translate the assignment part, either
        x = $foo->bar->baz[NN]
@@ -2352,7 +2381,13 @@ struct dwflpp
     die = translate_components (&pool, &tail, pc, components,
 				vardie, &die_mem, &attr_mem);
     if(!die)
-	throw semantic_error("Translation Failure");
+	{ 
+	  die = dwarf_formref_die (&attr_mem, vardie);
+          stringstream alternatives;
+          print_members(die,alternatives); 
+	  throw semantic_error("Translation failure : \n ALTERNATIVES [ " + alternatives.str() + " ] \n");
+	}
+
 
     /* Translate the assignment part, either
        x = $return->bar->baz[NN]

Re: [RFC updated PATCHES] Bug Translator 3016 : Error accessing members of anonymous structs / unions

[Masami Hiramatsu]

Hi Prerana,

Thank you for fixing!

I checked this patch worked well.
$ stap -e 'probe module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' -vp2
Pass 1: parsed user script and 45 library script(s) in 370usr/30sys/401real ms.
semantic error: struct/union 'children' is being accessed instead of a member of the struct/union: identifier 
'$port_dev' at <input>:1:67
Pass 2: analyzed script: 2 probe(s), 0 function(s), 0 embed(s), 0 global(s) in 160usr/1930sys/2092real ms.
Pass 2: analysis failed.  Try again with more '-v' (verbose) options.

Ack!

Prerna Saxena wrote:
> Hi Masami,
> Thanks for letting me know..
> It pointed me to an incorrect pointer reference that I had 
> overlooked...Its funny why this was never uncovered in all test-runs I 
> had on x86 & x86_64 :-)
> Here's the updated patch...with this, your test script correctly throws 
> a semantic error instead of causing SEGV.
> 
> Masami Hiramatsu wrote:
>> Hi Prerna,
>>
>> Prerna Saxena wrote:
>>> Agree, but making "die_mem" as the 6th arg will disturb the call flow.
>>> This is because in each recursive call, the 6th arg is overwritten by 
>>> a die which is pointed to by DW_AT_type attribute of the original 
>>> attr_mem. If die_mem is re-used as 6th arg instead of a new variable, 
>>> the old contents of die (in the parent recursive call) will also be 
>>> lost as both die & die_mem point to the same location, (whose 
>>> contents would be overwritten). This is not a problem if search in a 
>>> branch  has succeeded-- but in case a search path fails and a new 
>>> branch needs to be tried, this will deem it impossible.
>>>
>>> I agree with your concern about "temp_die_2" being local memory on 
>>> the stack which may be reused, so I've fixed it by copying the 
>>> contents of "temp_die_2" to "die_mem" in case of a successful match. 
>>> This should take care of memory errors.
>>
>> Hmm, you seems right, but the below command still cause SEGV.
>>
>> $ stap -e 'probe 
>> module("libsas").function("sas_ex_revalidate_domain"){print($port_dev->ex_dev->children)}' 
>> -vp2
>> Pass 1: parsed user script and 45 library script(s) in 
>> 370usr/30sys/404real ms.
>> Segmentation fault
>>
>> Would you run above command for testing & debugging?
>>
>> Thank you,
>>
> Thanks & regards,
> 

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com