usdt: Use `optional` to properly handle missing args

src/cc/usdt.h

@@ -17,28 +17,33 @@
 
 #include <string>
 #include <unordered_map>
+#include "vendor/optional.hpp"
 
 namespace USDT {
 
+using std::experimental::optional;
+using std::experimental::nullopt;
 class ArgumentParser;
 
 class Argument {
  private:
-  int arg_size_;
-  int constant_;
-  int deref_offset_;
-  std::string deref_ident_;
-  std::string register_name_;
+  optional<int> arg_size_;
+  optional<int> constant_;
+  optional<int> deref_offset_;
+  optional<std::string> deref_ident_;
+  optional<std::string> register_name_;
 
  public:
   Argument();
   ~Argument();
 
-  const std::string &deref_ident() const { return deref_ident_; }
-  const std::string &register_name() const { return register_name_; }
-  int arg_size() const { return arg_size_; }
-  int constant() const { return constant_; }
-  int deref_offset() const { return deref_offset_; }
+  int arg_size() const { return arg_size_.value_or(sizeof(void *)); }
+
+  const optional<std::string> &deref_ident() const { return deref_ident_; }
+  const optional<std::string> &register_name() const { return register_name_; }
+  const optional<int> constant() const { return constant_; }
+  const optional<int> deref_offset() const { return deref_offset_; }
+
   friend class ArgumentParser;
 };
 
@@ -49,8 +54,8 @@ class ArgumentParser {
  protected:
   virtual bool validate_register(const std::string &reg, int *reg_size) = 0;
 
-  ssize_t parse_number(ssize_t pos, int *number);
-  ssize_t parse_identifier(ssize_t pos, std::string *ident);
+  ssize_t parse_number(ssize_t pos, optional<int> *number);
+  ssize_t parse_identifier(ssize_t pos, optional<std::string> *ident);
   ssize_t parse_register(ssize_t pos, Argument *dest);
   ssize_t parse_expr(ssize_t pos, Argument *dest);
   ssize_t parse_1(ssize_t pos, Argument *dest);

src/cc/usdt_args.cc

@@ -13,26 +13,29 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-#include <unordered_map>
 #include "usdt.h"
+#include <unordered_map>
 
 namespace USDT {
 
-Argument::Argument() : arg_size_(0), constant_(0), deref_offset_(0) {}
-
+Argument::Argument() {}
 Argument::~Argument() {}
 
-ssize_t ArgumentParser::parse_number(ssize_t pos, int *number) {
+ssize_t ArgumentParser::parse_number(ssize_t pos, optional<int> *result) {
   char *endp;
-  *number = strtol(arg_ + pos, &endp, 0);
+  int number = strtol(arg_ + pos, &endp, 0);
+  if (endp > arg_ + pos)
+    *result = number;
   return endp - arg_;
 }
 
-ssize_t ArgumentParser::parse_identifier(ssize_t pos, std::string *ident) {
+ssize_t ArgumentParser::parse_identifier(ssize_t pos,
+                                         optional<std::string> *result) {
   if (isalpha(arg_[pos]) || arg_[pos] == '_') {
     ssize_t start = pos++;
     while (isalnum(arg_[pos]) || arg_[pos] == '_') pos++;
-    ident->assign(arg_ + start, pos - start);
+    if (pos - start)
+      result->emplace(arg_ + start, pos - start);
   }
   return pos;
 }
@@ -42,9 +45,17 @@ ssize_t ArgumentParser::parse_register(ssize_t pos, Argument *dest) {
   if (arg_[start] != '%')
     return -start;
   while (isalnum(arg_[pos])) pos++;
-  dest->register_name_.assign(arg_ + start, pos - start);
-  if (!validate_register(dest->register_name(), &dest->arg_size_))
+
+  std::string regname(arg_ + start, pos - start);
+  int regsize = 0;
+
+  if (!validate_register(regname, &regsize))
     return -start;
+
+  dest->register_name_ = regname;
+  if (!dest->arg_size_)
+    dest->arg_size_ = regsize;
+
   return pos;
 }
 
@@ -59,10 +70,11 @@ ssize_t ArgumentParser::parse_expr(ssize_t pos, Argument *dest) {
     pos = parse_number(pos, &dest->deref_offset_);
     if (arg_[pos] == '+') {
       pos = parse_identifier(pos + 1, &dest->deref_ident_);
-      if (dest->deref_ident().empty())
+      if (!dest->deref_ident_)
         return -pos;
     }
   } else {
+    dest->deref_offset_ = 0;
     pos = parse_identifier(pos, &dest->deref_ident_);
   }
 
@@ -78,9 +90,9 @@ ssize_t ArgumentParser::parse_expr(ssize_t pos, Argument *dest) {
 
 ssize_t ArgumentParser::parse_1(ssize_t pos, Argument *dest) {
   if (isdigit(arg_[pos]) || arg_[pos] == '-') {
-    int asize;
+    optional<int> asize;
     ssize_t m = parse_number(pos, &asize);
-    if (arg_[m] == '@') {
+    if (arg_[m] == '@' && asize) {
       dest->arg_size_ = asize;
       return parse_expr(m + 1, dest);
     }
@@ -132,8 +144,7 @@ bool ArgumentParser_x64::validate_register(const std::string &reg,
   auto it = registers_.find(reg);
   if (it == registers_.end())
     return false;
-  if (*reg_size == 0)
-    *reg_size = it->second;
+  *reg_size = it->second;
   return true;
 }
 }

tests/cc/test_usdt_args.cc

@@ -2,14 +2,30 @@
 #include "catch.hpp"
 #include "usdt.h"
 
+using std::experimental::optional;
+using std::experimental::nullopt;
+
 static void verify_register(USDT::ArgumentParser_x64 &parser, int arg_size,
-                            const std::string &register_name, int constant,
-                            int deref_offset, const std::string &deref_ident) {
+                            int constant) {
   USDT::Argument arg;
   REQUIRE(parser.parse(&arg));
   REQUIRE(arg.arg_size() == arg_size);
-  REQUIRE(arg.register_name() == register_name);
+
+  REQUIRE(arg.constant());
   REQUIRE(arg.constant() == constant);
+}
+
+static void verify_register(USDT::ArgumentParser_x64 &parser, int arg_size,
+                            const std::string &regname,
+                            optional<int> deref_offset = nullopt,
+                            optional<std::string> deref_ident = nullopt) {
+  USDT::Argument arg;
+  REQUIRE(parser.parse(&arg));
+  REQUIRE(arg.arg_size() == arg_size);
+
+  REQUIRE(arg.register_name());
+  REQUIRE(arg.register_name() == regname);
+
   REQUIRE(arg.deref_offset() == deref_offset);
   REQUIRE(arg.deref_ident() == deref_ident);
 }
@@ -22,17 +38,20 @@ TEST_CASE("test usdt argument parsing", "[usdt]") {
         "-4@global_max_action(%rip) "
         "8@24+mp_(%rip) ");
 
-    verify_register(parser, -4, "", 0, 0, "");
-    verify_register(parser, 8, "", 1234, 0, "");
-    verify_register(parser, 8, "%rdi", 0, 0, "");
-    verify_register(parser, 8, "%rax", 0, 0, "");
-    verify_register(parser, 8, "%rsi", 0, 0, "");
-    verify_register(parser, -8, "%rbx", 0, 0, "");
-    verify_register(parser, 4, "%r12", 0, 0, "");
-    verify_register(parser, 8, "%rbp", 0, -8, "");
-    verify_register(parser, 4, "%rax", 0, 0, "");
-    verify_register(parser, -4, "%rip", 0, 0, "global_max_action");
-    verify_register(parser, 8, "%rip", 0, 24, "mp_");
+    verify_register(parser, -4, 0);
+    verify_register(parser, 8, 1234);
+
+    verify_register(parser, 8, "%rdi");
+    verify_register(parser, 8, "%rax");
+    verify_register(parser, 8, "%rsi");
+    verify_register(parser, -8, "%rbx");
+    verify_register(parser, 4, "%r12");
+
+    verify_register(parser, 8, "%rbp", -8);
+    verify_register(parser, 4, "%rax", 0);
+
+    verify_register(parser, -4, "%rip", 0, std::string("global_max_action"));
+    verify_register(parser, 8, "%rip", 24, std::string("mp_"));
 
     REQUIRE(parser.done());
   }