PostfixExpression: Use signed integers in IntegerNode

[labath]

Summary

This is necessary to support parsing expressions like ".cfa -16 + ^", as
that format is used in breakpad STACK CFI expressions.

Since the PDB expressions use the same parser, this change will affect
them too, but I don't believe that should be a problem in practice. If
PDBs do contain the negative values, it's very likely that they are
intended to be parsed the same way, and if they don't, then it doesn't
matter.

In case that we do ever need to handle this differently, we can always
make the parser behavior customizable, or just use a different parser.

To make sure that the integer size is big enough for everyone, I switch
from using a (unsigned) 32-bit integer to a 64-bit (signed) one.

[amccarth]

This looks fine to me, and the rationale sounds right. I'll be curious to see if any other reviewers see a potential problem with this.

[clayborg]

include/lldb/Symbol/PostfixExpression.h

97 ↗

(On Diff #197320)

Do we want to try and use lldb_private::Scalar here? Then this could handle signed/unsigned ints, floats and anything else that comes along? Might need to be renamed "ScalarNode"? All of the type promotion stuff is already built into those classes.

[labath]

include/lldb/Symbol/PostfixExpression.h

97 ↗

(On Diff #197320)

I think that would be too generic for the current use case. For instance, that'd mean I'd have to figure out how to serialize the floats into a dwarf expression. I'm not sure if that's possible, but it doesn't seem terribly useful, as the purpose of these expressions is to compute addresses of stuff, and addresses are integral. If it turns out 64 bit signed int isn't enough, then I'd go for llvm::APSInt, but so far I don't see anything which would suggest that will ever be the case.

lldb/trunk/include/lldb/Symbol/PostfixExpression.h

public:
   static bool classof(const Node *node) {
     return node->GetKind() == InitialValue;
   }
 };
 
 /// A node representing an integer literal.
 class IntegerNode : public Node {
 public:
-  IntegerNode(uint32_t value) : Node(Integer), m_value(value) {}
+  IntegerNode(int64_t value) : Node(Integer), m_value(value) {}
 
-  uint32_t GetValue() const { return m_value; }
+  int64_t GetValue() const { return m_value; }
 
   static bool classof(const Node *node) { return node->GetKind() == Integer; }
 
 private:
-  uint32_t m_value;
+  int64_t m_value;
 };
 
 /// A node representing the value of a register with the given register number.
 /// The register kind (RegisterKind enum) used for the specifying the register
 /// number is implicit and assumed to be the same for all Register nodes in a
 /// given tree.
 class RegisterNode : public Node {
 public:

lldb/trunk/source/Symbol/PostfixExpression.cpp

if (auto op_type = GetUnaryOpType(token)) {
       if (stack.empty())
         return nullptr;
 
       Node *operand = stack.pop_back_val();
       stack.push_back(MakeNode<UnaryOpNode>(alloc, *op_type, *operand));
       continue;
     }
 
-    uint32_t value;
+    int64_t value;
     if (to_integer(token, value, 10)) {
       // token is integer literal
       stack.push_back(MakeNode<IntegerNode>(alloc, value));
       continue;
     }
 
     stack.push_back(MakeNode<SymbolNode>(alloc, token));
   }
public:
   using Visitor<>::Dispatch;
 
 private:
   void Visit(BinaryOpNode &binary, Node *&) override;
 
   void Visit(InitialValueNode &val, Node *&) override;
 
   void Visit(IntegerNode &integer, Node *&) override {
-    m_out_stream.PutHex8(DW_OP_constu);
-    m_out_stream.PutULEB128(integer.GetValue());
+    m_out_stream.PutHex8(DW_OP_consts);
+    m_out_stream.PutSLEB128(integer.GetValue());
     ++m_stack_depth;
   }
 
   void Visit(RegisterNode &reg, Node *&) override;
 
   void Visit(SymbolNode &symbol, Node *&) override {
     llvm_unreachable("Symbols should have been resolved by now!");
   }

lldb/trunk/unittests/Symbol/PostfixExpressionTest.cpp

TEST(PostfixExpression, Parse) {
   EXPECT_EQ("-(int(1), int(2))", ParseAndStringify("1 2 -"));
   EXPECT_EQ("@(int(1), int(2))", ParseAndStringify("1 2 @"));
   EXPECT_EQ("+(int(1), +(int(2), int(3)))", ParseAndStringify("1 2 3 + +"));
   EXPECT_EQ("+(+(int(1), int(2)), int(3))", ParseAndStringify("1 2 + 3 +"));
   EXPECT_EQ("^(int(1))", ParseAndStringify("1 ^"));
   EXPECT_EQ("^(^(int(1)))", ParseAndStringify("1 ^ ^"));
   EXPECT_EQ("^(+(int(1), ^(int(2))))", ParseAndStringify("1 2 ^ + ^"));
   EXPECT_EQ("-($foo, int(47))", ParseAndStringify("$foo 47 -"));
+  EXPECT_EQ("+(int(47), int(-42))", ParseAndStringify("47 -42 +"));
 
   EXPECT_EQ("nullptr", ParseAndStringify("+"));
   EXPECT_EQ("nullptr", ParseAndStringify("^"));
   EXPECT_EQ("nullptr", ParseAndStringify("1 +"));
   EXPECT_EQ("nullptr", ParseAndStringify("1 2 ^"));
   EXPECT_EQ("nullptr", ParseAndStringify("1 2 3 +"));
   EXPECT_EQ("nullptr", ParseAndStringify("^ 1"));
   EXPECT_EQ("nullptr", ParseAndStringify("+ 1 2"));
if (!DWARFExpression::PrintDWARFExpression(result, extractor, addr_size,
                                              /*location_expression*/ false)) {
     return "DWARF printing failed.";
   }
 
   return result.GetString();
 }
 
 TEST(PostfixExpression, ToDWARF) {
-  EXPECT_EQ("DW_OP_constu 0x0", ParseAndGenerateDWARF("0"));
+  EXPECT_EQ("DW_OP_consts +0", ParseAndGenerateDWARF("0"));
 
   EXPECT_EQ("DW_OP_breg1 +0", ParseAndGenerateDWARF("R1"));
 
   EXPECT_EQ("DW_OP_bregx 65 0", ParseAndGenerateDWARF("R65"));
 
   EXPECT_EQ("DW_OP_pick 0x00", ParseAndGenerateDWARF("INIT"));
 
   EXPECT_EQ("DW_OP_pick 0x00, DW_OP_pick 0x01, DW_OP_plus ",
             ParseAndGenerateDWARF("INIT INIT +"));
 
   EXPECT_EQ("DW_OP_breg1 +0, DW_OP_pick 0x01, DW_OP_plus ",
             ParseAndGenerateDWARF("R1 INIT +"));
 
-  EXPECT_EQ("DW_OP_constu 0x1, DW_OP_pick 0x01, DW_OP_deref , DW_OP_plus ",
+  EXPECT_EQ("DW_OP_consts +1, DW_OP_pick 0x01, DW_OP_deref , DW_OP_plus ",
             ParseAndGenerateDWARF("1 INIT ^ +"));
 
-  EXPECT_EQ("DW_OP_constu 0x4, DW_OP_constu 0x5, DW_OP_plus ",
+  EXPECT_EQ("DW_OP_consts +4, DW_OP_consts +5, DW_OP_plus ",
             ParseAndGenerateDWARF("4 5 +"));
 
-  EXPECT_EQ("DW_OP_constu 0x4, DW_OP_constu 0x5, DW_OP_minus ",
+  EXPECT_EQ("DW_OP_consts +4, DW_OP_consts +5, DW_OP_minus ",
             ParseAndGenerateDWARF("4 5 -"));
 
-  EXPECT_EQ("DW_OP_constu 0x4, DW_OP_deref ", ParseAndGenerateDWARF("4 ^"));
+  EXPECT_EQ("DW_OP_consts +4, DW_OP_deref ", ParseAndGenerateDWARF("4 ^"));
 
-  EXPECT_EQ("DW_OP_breg6 +0, DW_OP_constu 0x80, DW_OP_lit1 "
+  EXPECT_EQ("DW_OP_breg6 +0, DW_OP_consts +128, DW_OP_lit1 "
             ", DW_OP_minus , DW_OP_not , DW_OP_and ",
             ParseAndGenerateDWARF("R6 128 @"));
 }

lldb/trunk/unittests/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpressionTests.cpp

ASSERT_STREQ(expected_dwarf_expression.data(),
                result_dwarf_expression.GetString().data());
 }
 
 TEST(PDBFPOProgramToDWARFExpressionTests, SingleAssignmentRegisterRef) {
   CheckValidProgramTranslation("$T0 $ebp = ", "$T0", "DW_OP_breg6 +0");
 }
 
 TEST(PDBFPOProgramToDWARFExpressionTests, MultipleIndependentAssignments) {
-  CheckValidProgramTranslation("$T1 1 = $T0 0 =", "$T0", "DW_OP_constu 0x0");
+  CheckValidProgramTranslation("$T1 1 = $T0 0 =", "$T0", "DW_OP_consts +0");
 }
 
 TEST(PDBFPOProgramToDWARFExpressionTests, MultipleDependentAssignments) {
   CheckValidProgramTranslation(
       "$T1 $ebp 4 + = $T0 $T1 8 - 128 @ = ", "$T0",
-      "DW_OP_breg6 +0, DW_OP_constu 0x4, DW_OP_plus , DW_OP_constu 0x8, "
-      "DW_OP_minus , DW_OP_constu 0x80, DW_OP_lit1 , DW_OP_minus , DW_OP_not , "
+      "DW_OP_breg6 +0, DW_OP_consts +4, DW_OP_plus , DW_OP_consts +8, "
+      "DW_OP_minus , DW_OP_consts +128, DW_OP_lit1 , DW_OP_minus , DW_OP_not , "
       "DW_OP_and ");
 }
 
 TEST(PDBFPOProgramToDWARFExpressionTests, DependencyChain) {
   CheckValidProgramTranslation("$T1 0 = $T0 $T1 = $ebp $T0 =", "$ebp",
-                               "DW_OP_constu 0x0");
+                               "DW_OP_consts +0");
 }
 
 /// Invalid programs tests
 static void
 CheckInvalidProgramTranslation(llvm::StringRef fpo_program,
                                llvm::StringRef target_register_name) {
   // initial setup
   ArchSpec arch_spec("i686-pc-windows");