[PATCH] PR13475: Fix ARM SDT_V3 operand parsing

[PATCH] PR13475: Fix ARM SDT_V3 operand parsing

[Wade Farnsworth]

* Include regular expressions to parse ARM operands
* Add ARM register data
* Allow for whitespace in ARM operands containing []'s

Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
---
 tapsets.cxx |  135 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 116 insertions(+), 19 deletions(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index 716e23c..4215166 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -5028,10 +5028,51 @@ dwarf_derived_probe_group::emit_module_exit (systemtap_session& s)
   s.op->newline(-1) << "}";
 }
 
+/* ARM tokens may contain space chars, e.g., [REG, OFFSET] */
+void arm_tokenize(const string& str, vector<string>& tokens)
+{
+  // Skip delimiters at beginning.
+  string::size_type lastPos = str.find_first_not_of(" ", 0);
+  // Find first "non-delimiter".
+  string::size_type pos     = str.find_first_of(" ", lastPos);
+  string::size_type cpos;
+  string temp, sub, tok;
+  bool push = true;
+
+  while (pos != string::npos || lastPos != string::npos)
+    {
+      sub = str.substr(lastPos, pos - lastPos);
+      if ((cpos = sub.find('[')) != string::npos)
+        {
+          if (sub.find(']', cpos) != string::npos)
+            tokens.push_back(sub);
+          else
+            {
+              push = false;
+              temp = sub;
+            }
+        }
+      else if (sub.find(']') != string::npos)
+        {
+          push = true;
+          tokens.push_back(temp + " " + sub);
+        }
+      else if (push)
+        tokens.push_back(sub);
+      else
+        temp = temp + " " + sub;
+
+      // Skip delimiters.  Note the "not_of"
+      lastPos = str.find_first_not_of(" ", pos);
+      // Find next "non-delimiter"
+      pos = str.find_first_of(" ", lastPos);
+    }
+}
 
 struct sdt_kprobe_var_expanding_visitor: public var_expanding_visitor
 {
-  sdt_kprobe_var_expanding_visitor(const string & process_name,
+  sdt_kprobe_var_expanding_visitor(int elf_machine,
+				   const string & process_name,
 				   const string & provider_name,
 				   const string & probe_name,
 				   const string & arg_string,
@@ -5039,7 +5080,10 @@ struct sdt_kprobe_var_expanding_visitor: public var_expanding_visitor
     process_name (process_name), provider_name (provider_name), probe_name (probe_name),
     arg_count (arg_count)
   {
-    tokenize(arg_string, arg_tokens, " ");
+    if (elf_machine == EM_ARM)
+      arm_tokenize(arg_string, arg_tokens);
+    else
+      tokenize(arg_string, arg_tokens, " ");
     assert(arg_count <= 10);
   }
   const string & process_name;
@@ -5200,13 +5244,34 @@ struct sdt_uprobe_var_expanding_visitor: public var_expanding_visitor
       DRI ("%r13", 13, DI);
       DRI ("%r14", 14, DI);
       DRI ("%r15", 15, DI);
+    } else if (elf_machine == EM_ARM) {
+      DRI ("r0", 0, SI);
+      DRI ("r1", 1, SI);
+      DRI ("r2", 2, SI);
+      DRI ("r3", 3, SI);
+      DRI ("r4", 4, SI);
+      DRI ("r5", 5, SI);
+      DRI ("r6", 6, SI);
+      DRI ("r7", 7, SI);
+      DRI ("r8", 8, SI);
+      DRI ("r9", 9, SI);
+      DRI ("sl", 10, SI);
+      DRI ("fp", 11, SI);
+      DRI ("ip", 12, SI);
+      DRI ("sp", 13, SI);
+      DRI ("lr", 14, SI);
+      DRI ("pc", 15, SI);
     } else if (arg_count) {
       /* permit this case; just fall back to dwarf */
     }
 #undef DRI
 
     need_debug_info = false;
-    tokenize(arg_string, arg_tokens, " ");
+    if (elf_machine == EM_ARM)
+      arm_tokenize(arg_string, arg_tokens);
+    else
+      tokenize(arg_string, arg_tokens, " ");
+
     if (probe_type == uprobe3_type)
       assert(arg_count <= 12);
     else
@@ -5371,7 +5436,7 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
       // anyway.  With -mregnames, we could, if gcc somehow
       // communicated to us the presence of that option, but alas it
       // doesn't.  http://gcc.gnu.org/PR44995.
-      rc = regexp_match (asmarg, "^[i\\$][-]?[0-9][0-9]*$", matches);
+      rc = regexp_match (asmarg, "^[i\\$#][-]?[0-9][0-9]*$", matches);
       if (! rc)
         {
 	  string sn = matches[0].substr(1);
@@ -5420,11 +5485,12 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
         }
       // clip off leading |
       regnames = regnames.substr(1);
-      percent_regnames = percent_regnames.substr(1);
+      if (percent_regnames != "")
+          percent_regnames = percent_regnames.substr(1);
 
       // test for REGISTER
       // NB: Because PR11821, we must use percent_regnames here.
-      if (elf_machine == EM_PPC || elf_machine == EM_PPC64)
+      if (elf_machine == EM_PPC || elf_machine == EM_PPC64 || elf_machine == EM_ARM)
 	rc = regexp_match (asmarg, string("^(")+regnames+string(")$"), matches);
       else
 	rc = regexp_match (asmarg, string("^(")+percent_regnames+string(")$"), matches);
@@ -5473,22 +5539,34 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
           // invalid register name, fall through
         }
 
+      int reg, offset1;
       // test for OFFSET(REGISTER) where OFFSET is +-N+-N+-N
       // NB: Despite PR11821, we can use regnames here, since the parentheses
       // make things unambiguous. (Note: gdb/stap-probe.c also parses this)
-      rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+      // On ARM test for [REGISTER, OFFSET]
+      if (elf_machine == EM_ARM)
+        {
+          rc = regexp_match (asmarg, string("^\\[(")+regnames+string("), #([+-]?[0-9]+)([+-][0-9]*)?([+-][0-9]*)?\\]$"), matches);
+          reg = 1;
+          offset1 = 2;
+        }
+      else
+        {
+          rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+          reg = 4;
+          offset1 = 1;
+        }
       if (! rc)
         {
           string regname;
           int64_t disp = 0;
 
-
-          if (matches[4].length())
-            regname = matches[4];
+          if (matches[reg].length())
+            regname = matches[reg];
           if (dwarf_regs.find (regname) == dwarf_regs.end())
             goto not_matched;
 
-          for (int i=1; i <= 3; i++)
+          for (int i=offset1; i <= (offset1 + 2); i++)
             if (matches[i].length())
               try
                 {
@@ -5995,11 +6073,19 @@ sdt_query::handle_probe_entry()
   Dwarf_Addr bias;
   Elf* elf = dwfl_module_getelf (dw.mod_info->mod, &bias);
 
+  /* Figure out the architecture of this particular ELF file.
+     The dwarfless register-name mappings depend on it. */
+  GElf_Ehdr ehdr_mem;
+  GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
+  if (em == 0) { dwfl_assert ("dwfl_getehdr", dwfl_errno()); }
+  int elf_machine = em->e_machine;
+
   if (have_kprobe())
     {
       convert_probe(new_base);
       // Expand the local variables in the probe body
-      sdt_kprobe_var_expanding_visitor svv (module_val,
+      sdt_kprobe_var_expanding_visitor svv (elf_machine,
+					    module_val,
 					    provider_name,
 					    probe_name,
 					    arg_string,
@@ -6008,12 +6094,6 @@ sdt_query::handle_probe_entry()
     }
   else
     {
-      /* Figure out the architecture of this particular ELF file.
-	 The dwarfless register-name mappings depend on it. */
-      GElf_Ehdr ehdr_mem;
-      GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
-      if (em == 0) { dwfl_assert ("dwfl_getehdr", dwfl_errno()); }
-      int elf_machine = em->e_machine;
       sdt_uprobe_var_expanding_visitor svv (sess, elf_machine,
 					    module_val,
 					    provider_name,
@@ -6191,10 +6271,27 @@ sdt_query::setup_note_probe_entry (int type, const char *data, size_t len)
       return;
   arg_string = args;
 
+
+  /* ARM args may contain space chars, e.g. [REG, OFFSET] */
+  GElf_Ehdr ehdr_mem;
+  GElf_Ehdr* em = gelf_getehdr (elf, &ehdr_mem);
+  if (em == 0) { dwfl_assert ("dwfl_getehdr", dwfl_errno()); }
+  int elf_machine = em->e_machine;
+
+  bool incr_count = true;
   arg_count = 0;
   for (unsigned i = 0; i < arg_string.length(); i++)
-    if (arg_string[i] == ' ')
+    {
+    if (elf_machine == EM_ARM)
+      {
+       if (arg_string[i] == '[')
+         incr_count = false;
+       else if (arg_string[i] == ']')
+         incr_count = true;
+      }
+    if (incr_count && arg_string[i] == ' ')
       arg_count += 1;
+    }
   if (arg_string.length() != 0)
     arg_count += 1;
   
-- 
1.7.0.4

Re: [PATCH] PR13475: Fix ARM SDT_V3 operand parsing

[Josh Stone]

On 03/08/2012 06:53 AM, Wade Farnsworth wrote:
> * Allow for whitespace in ARM operands containing []'s

IIRC, this is sort of a latent issue on all archs.  While the compiler
tends not to add spaces on e.g. x86, SDT probes in hand-written asm
could very well have spaces in the arguments.  The only real separator
we have is the '@' in each SIZE@LOCATION.  I think Roland chose '@'
specifically for the belief that it wouldn't ever appear in the actual
location asm string.

I'm not sure if regex matching would make this easier or not, but it's
something like:  ([+-]?\d+@[^@]+?)(\s+[+-]?\d+@[^@]+?)*

Or just iteratively, find('@'), rfind(' '), split, repeat.

The actual handling for the location will always be arch specific, of
course, like dealing with ARM's [].


Josh

Re: [PATCH] PR13475: Fix ARM SDT_V3 operand parsing

[Wade Farnsworth]

Josh Stone wrote:
> On 03/08/2012 06:53 AM, Wade Farnsworth wrote:
>> * Allow for whitespace in ARM operands containing []'s
>
> IIRC, this is sort of a latent issue on all archs.  While the compiler
> tends not to add spaces on e.g. x86, SDT probes in hand-written asm
> could very well have spaces in the arguments.  The only real separator
> we have is the '@' in each SIZE@LOCATION.  I think Roland chose '@'
> specifically for the belief that it wouldn't ever appear in the actual
> location asm string.
>
> I'm not sure if regex matching would make this easier or not, but it's
> something like:  ([+-]?\d+@[^@]+?)(\s+[+-]?\d+@[^@]+?)*
>
> Or just iteratively, find('@'), rfind(' '), split, repeat.
>
> The actual handling for the location will always be arch specific, of
> course, like dealing with ARM's [].
>
>

It appears that operand parsing on ARM fails with V2 and V1 probes as 
well, which, as I understand, don't have the SIZE@ notation.  So your 
suggestion won't work in the generic case.

With that in mind, It may be beneficial to identify where the 
non-delimiting whitespace may occur.  On ARM, I only receive 
non-delimiting whitespace after a comma.  I don't believe that any 
architecture could have a comma as the last character of a token 
(correct me if I'm wrong), so it would be simple enough to implement a 
version of tokenize() that detects a ", " sequence as non-delimiting.

Does this sound like a reasonable approach?  Would there be any other 
such exceptions that we should be detecting?

Thanks,

Wade

[PATCH v2] PR13475: Fix ARM SDT_V3 operand parsing

[Wade Farnsworth]

* Include regular expressions to parse ARM operands.
* Add ARM register data.
* Allow for whitespace in operands that follow comma characters.

Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
---

v2: - Changed arm_tokenize() to arg_string_split() which is used on
      every architecture.  Token delimiters are space characters not 
      following a comma character.  These changes are per my previous email.

 tapsets.cxx |   71 +++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index ccabc67..170c22d 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -5102,6 +5102,24 @@ dwarf_derived_probe_group::emit_module_exit (systemtap_session& s)
   s.op->newline(-1) << "}";
 }
 
+static void arg_string_split(const string& str, vector<string>& tokens)
+{
+  string::size_type pos;
+  string::size_type lastPos = str.find_first_not_of(" ", 0);
+  while (lastPos != string::npos)
+   {
+     pos = str.find(" ", lastPos);
+     while (str[pos - 1] == ',')
+       {
+         // ignore any whitespace following ','
+         pos = str.find_first_not_of(" ", pos);
+         pos = str.find(" ", pos);
+       }
+
+     tokens.push_back(str.substr(lastPos, pos - lastPos));
+     lastPos = str.find_first_not_of(" ", pos);
+   }
+}
 
 struct sdt_kprobe_var_expanding_visitor: public var_expanding_visitor
 {
@@ -5113,7 +5131,7 @@ struct sdt_kprobe_var_expanding_visitor: public var_expanding_visitor
     process_name (process_name), provider_name (provider_name), probe_name (probe_name),
     arg_count (arg_count)
   {
-    tokenize(arg_string, arg_tokens, " ");
+    arg_string_split(arg_string, arg_tokens);
     assert(arg_count <= 10);
   }
   const string & process_name;
@@ -5274,13 +5292,30 @@ struct sdt_uprobe_var_expanding_visitor: public var_expanding_visitor
       DRI ("%r13", 13, DI);
       DRI ("%r14", 14, DI);
       DRI ("%r15", 15, DI);
+    } else if (elf_machine == EM_ARM) {
+      DRI ("r0", 0, SI);
+      DRI ("r1", 1, SI);
+      DRI ("r2", 2, SI);
+      DRI ("r3", 3, SI);
+      DRI ("r4", 4, SI);
+      DRI ("r5", 5, SI);
+      DRI ("r6", 6, SI);
+      DRI ("r7", 7, SI);
+      DRI ("r8", 8, SI);
+      DRI ("r9", 9, SI);
+      DRI ("sl", 10, SI);
+      DRI ("fp", 11, SI);
+      DRI ("ip", 12, SI);
+      DRI ("sp", 13, SI);
+      DRI ("lr", 14, SI);
+      DRI ("pc", 15, SI);
     } else if (arg_count) {
       /* permit this case; just fall back to dwarf */
     }
 #undef DRI
 
     need_debug_info = false;
-    tokenize(arg_string, arg_tokens, " ");
+    arg_string_split(arg_string, arg_tokens);
     if (probe_type == uprobe3_type)
       assert(arg_count <= 12);
     else
@@ -5445,7 +5480,7 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
       // anyway.  With -mregnames, we could, if gcc somehow
       // communicated to us the presence of that option, but alas it
       // doesn't.  http://gcc.gnu.org/PR44995.
-      rc = regexp_match (asmarg, "^[i\\$][-]?[0-9][0-9]*$", matches);
+      rc = regexp_match (asmarg, "^[i\\$#][-]?[0-9][0-9]*$", matches);
       if (! rc)
         {
 	  string sn = matches[0].substr(1);
@@ -5494,11 +5529,12 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
         }
       // clip off leading |
       regnames = regnames.substr(1);
-      percent_regnames = percent_regnames.substr(1);
+      if (percent_regnames != "")
+          percent_regnames = percent_regnames.substr(1);
 
       // test for REGISTER
       // NB: Because PR11821, we must use percent_regnames here.
-      if (elf_machine == EM_PPC || elf_machine == EM_PPC64)
+      if (elf_machine == EM_PPC || elf_machine == EM_PPC64 || elf_machine == EM_ARM)
 	rc = regexp_match (asmarg, string("^(")+regnames+string(")$"), matches);
       else
 	rc = regexp_match (asmarg, string("^(")+percent_regnames+string(")$"), matches);
@@ -5547,22 +5583,33 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
           // invalid register name, fall through
         }
 
+      int reg, offset1;
       // test for OFFSET(REGISTER) where OFFSET is +-N+-N+-N
       // NB: Despite PR11821, we can use regnames here, since the parentheses
       // make things unambiguous. (Note: gdb/stap-probe.c also parses this)
-      rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+      // On ARM test for [REGISTER, OFFSET]
+     if (elf_machine == EM_ARM)
+       {
+         rc = regexp_match (asmarg, string("^\\[(")+regnames+string("), #([+-]?[0-9]+)([+-][0-9]*)?([+-][0-9]*)?\\]$"), matches);
+         reg = 1;
+         offset1 = 2;
+       }
+     else
+       {
+         rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+         reg = 4;
+         offset1 = 1;
+       }
       if (! rc)
         {
           string regname;
           int64_t disp = 0;
-
-
-          if (matches[4].length())
-            regname = matches[4];
+          if (matches[reg].length())
+            regname = matches[reg];
           if (dwarf_regs.find (regname) == dwarf_regs.end())
             goto not_matched;
 
-          for (int i=1; i <= 3; i++)
+          for (int i=offset1; i <= (offset1 + 2); i++)
             if (matches[i].length())
               try
                 {
@@ -6269,7 +6316,7 @@ sdt_query::setup_note_probe_entry (int type, const char *data, size_t len)
 
   arg_count = 0;
   for (unsigned i = 0; i < arg_string.length(); i++)
-    if (arg_string[i] == ' ')
+    if (arg_string[i] == ' ' && (i == 0 || arg_string[i - 1] != ','))
       arg_count += 1;
   if (arg_string.length() != 0)
     arg_count += 1;
-- 
1.7.0.4

[PATCH v3 0/2] PR13475: ARM SDT fixes

[Wade Farnsworth]

Fix SDT_V3 operand parsing and update sdt_misc tests for ARM.

Wade Farnsworth (2):
  PR13475: Fix ARM SDT_V3 operand parsing
  Update sdt_misc.exp testcase for ARM

 tapsets.cxx                           |   80 ++++++++++++++++++++++++++------
 testsuite/systemtap.base/sdt_misc.exp |   43 +++++++++++++++---
 2 files changed, 101 insertions(+), 22 deletions(-)

[PATCH v3 1/2] PR13475: Fix ARM SDT_V3 operand parsing

[Wade Farnsworth]

* Include regular expressions to parse ARM operands
* Add ARM register data
* Allow for whitespace in ARM operands containing []'s

Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
---
 tapsets.cxx |   80 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/tapsets.cxx b/tapsets.cxx
index ccabc67..97cedc0 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -5102,6 +5102,24 @@ dwarf_derived_probe_group::emit_module_exit (systemtap_session& s)
   s.op->newline(-1) << "}";
 }
 
+static void sdt_v3_tokenize(const string& str, vector<string>& tokens)
+{
+  string::size_type pos;
+  string::size_type lastPos = str.find_first_not_of(" ", 0);
+  string::size_type nextAt = str.find("@", lastPos);
+  while (lastPos != string::npos)
+   {
+     pos = nextAt + 1;
+     nextAt = str.find("@", pos);
+     if (nextAt == string::npos)
+       pos = string::npos;
+     else
+       pos = str.rfind(" ", nextAt);
+
+     tokens.push_back(str.substr(lastPos, pos - lastPos));
+     lastPos = str.find_first_not_of(" ", pos);
+   }
+}
 
 struct sdt_kprobe_var_expanding_visitor: public var_expanding_visitor
 {
@@ -5274,17 +5292,39 @@ struct sdt_uprobe_var_expanding_visitor: public var_expanding_visitor
       DRI ("%r13", 13, DI);
       DRI ("%r14", 14, DI);
       DRI ("%r15", 15, DI);
+    } else if (elf_machine == EM_ARM) {
+      DRI ("r0", 0, SI);
+      DRI ("r1", 1, SI);
+      DRI ("r2", 2, SI);
+      DRI ("r3", 3, SI);
+      DRI ("r4", 4, SI);
+      DRI ("r5", 5, SI);
+      DRI ("r6", 6, SI);
+      DRI ("r7", 7, SI);
+      DRI ("r8", 8, SI);
+      DRI ("r9", 9, SI);
+      DRI ("sl", 10, SI);
+      DRI ("fp", 11, SI);
+      DRI ("ip", 12, SI);
+      DRI ("sp", 13, SI);
+      DRI ("lr", 14, SI);
+      DRI ("pc", 15, SI);
     } else if (arg_count) {
       /* permit this case; just fall back to dwarf */
     }
 #undef DRI
 
     need_debug_info = false;
-    tokenize(arg_string, arg_tokens, " ");
     if (probe_type == uprobe3_type)
-      assert(arg_count <= 12);
+      {
+        sdt_v3_tokenize(arg_string, arg_tokens);
+        assert(arg_count <= 12);
+      }
     else
-      assert(arg_count <= 10);
+      {
+        tokenize(arg_string, arg_tokens, " ");
+        assert(arg_count <= 10);
+      }
   }
 
   systemtap_session& session;
@@ -5445,7 +5485,7 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
       // anyway.  With -mregnames, we could, if gcc somehow
       // communicated to us the presence of that option, but alas it
       // doesn't.  http://gcc.gnu.org/PR44995.
-      rc = regexp_match (asmarg, "^[i\\$][-]?[0-9][0-9]*$", matches);
+      rc = regexp_match (asmarg, "^[i\\$#][-]?[0-9][0-9]*$", matches);
       if (! rc)
         {
 	  string sn = matches[0].substr(1);
@@ -5494,11 +5534,12 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
         }
       // clip off leading |
       regnames = regnames.substr(1);
-      percent_regnames = percent_regnames.substr(1);
+      if (percent_regnames != "")
+          percent_regnames = percent_regnames.substr(1);
 
       // test for REGISTER
       // NB: Because PR11821, we must use percent_regnames here.
-      if (elf_machine == EM_PPC || elf_machine == EM_PPC64)
+      if (elf_machine == EM_PPC || elf_machine == EM_PPC64 || elf_machine == EM_ARM)
 	rc = regexp_match (asmarg, string("^(")+regnames+string(")$"), matches);
       else
 	rc = regexp_match (asmarg, string("^(")+percent_regnames+string(")$"), matches);
@@ -5547,22 +5588,33 @@ sdt_uprobe_var_expanding_visitor::visit_target_symbol_arg (target_symbol *e)
           // invalid register name, fall through
         }
 
+      int reg, offset1;
       // test for OFFSET(REGISTER) where OFFSET is +-N+-N+-N
       // NB: Despite PR11821, we can use regnames here, since the parentheses
       // make things unambiguous. (Note: gdb/stap-probe.c also parses this)
-      rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+      // On ARM test for [REGISTER, OFFSET]
+     if (elf_machine == EM_ARM)
+       {
+         rc = regexp_match (asmarg, string("^\\[(")+regnames+string("), #([+-]?[0-9]+)([+-][0-9]*)?([+-][0-9]*)?\\]$"), matches);
+         reg = 1;
+         offset1 = 2;
+       }
+     else
+       {
+         rc = regexp_match (asmarg, string("^([+-]?[0-9]*)([+-][0-9]*)?([+-][0-9]*)?[(](")+regnames+string(")[)]$"), matches);
+         reg = 4;
+         offset1 = 1;
+       }
       if (! rc)
         {
           string regname;
           int64_t disp = 0;
-
-
-          if (matches[4].length())
-            regname = matches[4];
+          if (matches[reg].length())
+            regname = matches[reg];
           if (dwarf_regs.find (regname) == dwarf_regs.end())
             goto not_matched;
 
-          for (int i=1; i <= 3; i++)
+          for (int i=offset1; i <= (offset1 + 2); i++)
             if (matches[i].length())
               try
                 {
@@ -6269,10 +6321,8 @@ sdt_query::setup_note_probe_entry (int type, const char *data, size_t len)
 
   arg_count = 0;
   for (unsigned i = 0; i < arg_string.length(); i++)
-    if (arg_string[i] == ' ')
+    if (arg_string[i] == '@')
       arg_count += 1;
-  if (arg_string.length() != 0)
-    arg_count += 1;
   
   GElf_Addr base_ref;
   if (gelf_getclass (elf) == ELFCLASS32)
-- 
1.7.0.4

[PATCH v3 2/2] Update sdt_misc.exp testcase for ARM

[Wade Farnsworth]

* Omit -m64 switch for wildcard tests, as ARM does not implement this
switch
* Mark a subset of the V1 and V2 tests as XFAIL, as they are known to fail
due to broken operand parsing on ARM.  Note that this brokenness does
not affect V3.

Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
---
 testsuite/systemtap.base/sdt_misc.exp |   43 +++++++++++++++++++++++++++-----
 1 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/testsuite/systemtap.base/sdt_misc.exp b/testsuite/systemtap.base/sdt_misc.exp
index 480c4c3..a128a34 100644
--- a/testsuite/systemtap.base/sdt_misc.exp
+++ b/testsuite/systemtap.base/sdt_misc.exp
@@ -106,7 +106,11 @@ catch {close}; catch {wait}
 if {$ok == 6} {
     pass "$test $pbtype_mssg"
 } else {
-    fail "$test ($ok) $pbtype_mssg"
+    if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && $pbtype_mssg == "V2_uprobe"} {
+        xfail "$test ($ok) $pbtype_mssg"
+    } else {
+        fail "$test ($ok) $pbtype_mssg"
+    }
 }
 
 # 2. Test attaching to a running process
@@ -147,7 +151,11 @@ catch {close}; catch {wait}
 if {$ok == 6} {
     pass "$test $pbtype_mssg attach"
 } else {
-    fail "$test ($ok) $pbtype_mssg attach"
+    if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && $pbtype_mssg == "V2_uprobe"} {
+        xfail "$test ($ok) $pbtype_mssg attach"
+    } else {
+        fail "$test ($ok) $pbtype_mssg attach"
+    }
 }
 
 set ok 0
@@ -197,9 +205,18 @@ set type_pedantic_mssg [lindex $type_pedantic_mssgs $p]
 set type_flags "[sdt_includes] additional_flags=-Wall additional_flags=-Werror"
 set type_flags "$type_flags additional_flags=-I. $pbtype_flag"
 set type_flags "$type_flags [lindex $extra_type_flags $j] [lindex $type_pedantics $p] [lindex $has_long_long $j]"
-if {![regexp "^i.86$" $::tcl_platform(machine)]} {
+if {![regexp "^(i.86|arm.*)$" $::tcl_platform(machine)]} {
     set type_flags "$type_flags additional_flags=-m64"
 }
+
+# On ARM these tests are known to fail with timeout for V1 and V2 probes.
+# Skip them altogether to speed execution.
+if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && ($pbtype_mssg == "V1_uprobe" || $pbtype_mssg == "V2_kprobe" || $pbtype_mssg == "V2_uprobe")} {
+    xfail "$test types $pbtype_mssg $extra_type_mssg $type_pedantic_mssg"
+    continue
+}
+
+
 set res [target_compile $srcdir/$subdir/sdt_types.c sdt_types.x executable $type_flags]
 if { $res != "" } {
     verbose "target_compile failed: $res" 2
@@ -289,7 +306,11 @@ catch {close}; catch {wait}
 if {$ok == 6} {
     pass "$test shared $pbtype_mssg"
 } else {
-    fail "$test ($ok) shared $pbtype_mssg"
+    if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && $pbtype_mssg == "V2_uprobe"} {
+        xfail "$test ($ok) shared $pbtype_mssg"
+    } else {
+        fail "$test ($ok) shared $pbtype_mssg"
+    }
 }
 
 # 5. Test attaching to a running process with markers in a shared object
@@ -338,7 +359,11 @@ catch {close}; catch {wait}
 if {$ok == 6} {
     pass "$test $pbtype_mssg shared attach"
 } else {
-    fail "$test ($ok) $pbtype_mssg shared attach"
+    if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && $pbtype_mssg == "V2_uprobe"} {
+       xfail "$test ($ok) $pbtype_mssg shared attach"
+    } else {
+       fail "$test ($ok) $pbtype_mssg shared attach"
+    }
 }
 
 set ok 0
@@ -388,7 +413,7 @@ expect {
 
 catch {close}; catch {wait}
 
-if { $ok == 51 || $ok == 48 || (($ok == 43 || $ok == 40 ) && [regexp "^(i.86)$" $::tcl_platform(machine)])} {
+if { $ok == 51 || $ok == 48 || (($ok == 43 || $ok == 40 ) && [regexp "^(i.86|arm.*)$" $::tcl_platform(machine)])} {
     pass "$test wildcard $pbtype_mssg"
 } else {
     fail "$test wildcard ($ok) $pbtype_mssg"
@@ -423,7 +448,11 @@ catch {close}; catch {wait}
 if {$ok == 1} {
     pass "$test $pbtype_mssg --types"
 } else {
-    fail "$test ($ok) $pbtype_mssg --types"
+    if {[regexp "^(arm.*)$" $::tcl_platform(machine)] && $pbtype_mssg == "V2_uprobe"} {
+        xfail "$test ($ok) $pbtype_mssg --types"
+    } else {
+        fail "$test ($ok) $pbtype_mssg --types"
+    }
 }
 
 } ; # end pbtype_flags
-- 
1.7.0.4

Re: [PATCH v3 1/2] PR13475: Fix ARM SDT_V3 operand parsing

[Mark Wielaard]

On Wed, Mar 28, 2012 at 07:46:16AM -0700, Wade Farnsworth wrote:
> * Include regular expressions to parse ARM operands
> * Add ARM register data
> * Allow for whitespace in ARM operands containing []'s
> 
> Signed-off-by: Wade Farnsworth <wade_farnsworth@mentor.com>
> ---
>  tapsets.cxx |   80 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 65 insertions(+), 15 deletions(-)
> 
> diff --git a/tapsets.cxx b/tapsets.cxx
> index ccabc67..97cedc0 100644
> --- a/tapsets.cxx
> +++ b/tapsets.cxx
> @@ -6269,10 +6321,8 @@ sdt_query::setup_note_probe_entry (int type, const char *data, size_t len)
>  
>    arg_count = 0;
>    for (unsigned i = 0; i < arg_string.length(); i++)
> -    if (arg_string[i] == ' ')
> +    if (arg_string[i] == '@')
>        arg_count += 1;
> -  if (arg_string.length() != 0)
> -    arg_count += 1;
>    

This part of the patch causes the following regression:
FAIL: sdt_misc asm (0) asm

spawn /usr/local/install/systemtap/bin/stap -c /usr/local/build/systemtap-obj/te
stsuite/sdt_asm.x /home/mark/src/systemtap/testsuite/systemtap.base/sdt_asm.stp 
/usr/local/build/systemtap-obj/testsuite/sdt_asm.x
WARNING: cannot find module /usr/local/build/systemtap-obj/testsuite/sdt_asm.x d
ebuginfo: No DWARF information found
semantic error: while resolving probe point: identifier 'process' at /home/mark/
src/systemtap/testsuite/systemtap.base/sdt_asm.stp:1:7
        source: probe process(@1).mark("a") 
                      ^

semantic error: no match

This is because this is a hand-written assembly probe.
And http://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation says:
"For compiler-generated code, each argument will be of the form N@OP.
For hand-written assembly, or for inline assembly in C or C++, the initial
N@ may be missing. If N is present, it describes the size of the argument.
[...] If N is omitted, the argument size is the natural size of the operand;
usually this is the size of the register or the word size of the machine.
In this case, the signedness is ambiguous."

So it seems we need to properly parse the arguments here and not just
depend on seeing a '@' or ' ' char.

I filed http://sourceware.org/bugzilla/show_bug.cgi?id=13934

Cheers,

Mark