This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [RFC updated PATCH 1/2] Bug Translator 3016 : Error accessing members of anonymous structs / unions
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
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]