Add DWARFFormValue::getAsSignedConstant().

[friss]

Summary

This will be used to dump some attributes in a natural (decimal) form.

It's not easy to come up with some code that would generate signed entities
in all the supported forms, thus I'm adding a unittest for the functionality.
Most of the test is actually boilerplate dummy objects that are required to
parse a DWARFFormValue.

[dblaikie]

lib/DebugInfo/DWARFFormValue.cpp

554

hmm - why is udata considered "not signed". It's specifically saying that the value is positive, so should be returned successfully that way, no?

Arguable, I suppose - but I wouldn't put it past implementations to return positive data in _udata to reduce the number of bytes needed to encode it.

unittests/DebugInfo/DWARFFormValueTest.cpp

103

Does this need tests for any other cases? (these tests would all pass with "getAsSignedConstant()" implemented as a single "return -1")

[friss]

lib/DebugInfo/DWARFFormValue.cpp

554

Right. This check wasn't there at first and I added it to prevent issues with values > LONG_MAX being returned as negative numbers. Given how we'll use the method this isn't really an issue.

Note that getAsUnsignedConstant rejects FORM_sdata. It has more reasons to do so, but along the same line of thought, it should maybe return a value if he sdata is in fact positive?

unittests/DebugInfo/DWARFFormValueTest.cpp

103

My main concern was to check that I got the sign extension right for all the FORM_data*. Some other cases can of course be added. Do you have any specific limit tests in mind that would be interesting, or just adding a few positive and negative values to the test would do it for you?

[dblaikie]

lib/DebugInfo/DWARFFormValue.cpp

554

I believe the right thing would be for the value to be printed as signed/unsigned based on udata/sdata - or based on context (form kind, etc) if it's dataN. Not sure how convenient that is to implement.

unittests/DebugInfo/DWARFFormValueTest.cpp

103

Yeah, I'd probably just try testing a few other values (potentially replacing the 4 values tested here with more variety - or at least some values in the middle of negatives/positive values, etc)... but maybe it doesn't matter. Up to you.

[samsonov]

unittests/DebugInfo/DWARFFormValueTest.cpp

10

Unittests may only include headers from include/llvm (i.e., public headers). If we want to add unittests that instantiate DWARFContext, we should make corresponding header public.

[friss]

lib/DebugInfo/DWARFFormValue.cpp

554

But here the question isn't even about printing, but only about accepting udata in getAsSignedConstant() and sdata in getAsUnsignedConstant(). Would you prefer the values to be accepted if they fit into the output range and return None otherwise?

unittests/DebugInfo/DWARFFormValueTest.cpp

10

I was actually wondering about this, but I found one prior example of including ../lib/. I'm not sure what the best way forward is for this, if we make the Context public, then everything gets public. Not that I'm opposed to that, it would make some of my planned work easier, and I might propose that in the near future.

What would people think about making the whole DWARF stuff a pubilc interface to libDebugInfo ? (Note that right now this is just to get some testing for this very small feature added just to be able to print AT_lower_bound as a signed decimal... I was already embarrassed by the amount of Dummy* instantiations to get my small test running, but getting the whole interface made public just for that seems a bit too much, doesn't it?)

[dblaikie]

lib/DebugInfo/DWARFFormValue.cpp

554

Well, except that getAs(Un)signedConstant is used for dumping (it's also used for other stuff - like getting indexes into the file table... - anything else?).

I think it'd be perfectly reasonable if someone used DW_FORM_sdata for DW_AT_decl_line, to print the value appropriately signed, rather than printing hex bytes. Seems like a good/legible thing to do, even if the input is nonsense.

[samsonov]

unittests/DebugInfo/DWARFFormValueTest.cpp

10

I don't really know. Certainly, this set up is an overkill for a small unit test you have. Testing isolated components of DWARF parser are hard because of all the relationship between various sections. However, we can expose DWARFContext.h in public headers somewhere under include/llvm/DebugInfo/DWARF, then create a DWARFTestContext in unittests/, and reuse it in various unit tests. I'm not sure if it would be convenient, or we'd actually need that many unit tests.

[friss]

unittests/DebugInfo/DWARFFormValueTest.cpp

10

But if we put DWARFContext.h in the public headers, we have to put (nearly) everything else. Just look at the list of includes at the beginning of DWARFContext.h.

[samsonov]

unittests/DebugInfo/DWARFFormValueTest.cpp

10

Sure, I understand this and therefore feel reluctant about this shift. We can do this, but only if it allows us to write a lot of good and helpful unit-tests. I don't yet see how unit-testing a DWARF parser routines can be made easily.

include/llvm/DebugInfo/DWARFFormValue.h

public:
   bool isInlinedCStr() const {
     return Value.data != nullptr && Value.data == (const uint8_t*)Value.cstr;
   }
 
   /// getAsFoo functions below return the extracted value as Foo if only
   /// DWARFFormValue has form class is suitable for representing Foo.
   Optional<uint64_t> getAsReference(const DWARFUnit *U) const;
   Optional<uint64_t> getAsUnsignedConstant() const;
+  Optional<int64_t> getAsSignedConstant() const;
   Optional<const char *> getAsCString(const DWARFUnit *U) const;
   Optional<uint64_t> getAsAddress(const DWARFUnit *U) const;
   Optional<uint64_t> getAsSectionOffset() const;
   Optional<ArrayRef<uint8_t>> getAsBlock() const;
 
   bool skipValue(DataExtractor debug_info_data, uint32_t *offset_ptr,
                  const DWARFUnit *u) const;
   static bool skipValue(uint16_t form, DataExtractor debug_info_data,

lib/DebugInfo/DWARFFormValue.cpp

 
 Optional<uint64_t> DWARFFormValue::getAsUnsignedConstant() const {
   if ((!isFormClass(FC_Constant) && !isFormClass(FC_Flag))
       || Form == DW_FORM_sdata)
     return None;
   return Value.uval;
 }
 
+Optional<int64_t> DWARFFormValue::getAsSignedConstant() const {
+  if ((!isFormClass(FC_Constant) && !isFormClass(FC_Flag))
+      || Form == DW_FORM_udata)
+    return None;
+  switch (Form) {
+  case DW_FORM_data4:
+    return (int32_t)Value.uval;
+  case DW_FORM_data2:
+    return (int16_t)Value.uval;
+  case DW_FORM_data1:
+    return (int8_t)Value.uval;
+  case DW_FORM_sdata:
+  case DW_FORM_data8:
+  default:
+    return Value.sval;
+  }
+}
+
 Optional<ArrayRef<uint8_t>> DWARFFormValue::getAsBlock() const {
   if (!isFormClass(FC_Block) && !isFormClass(FC_Exprloc))
     return None;
   return ArrayRef<uint8_t>(Value.data, Value.uval);
 }
 

unittests/DebugInfo/DWARFFormValueTest.cpp

 //===- llvm/unittest/DebugInfo/DWARFFormValueTest.cpp ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "../lib/DebugInfo/DWARFContext.h"
 #include "llvm/DebugInfo/DWARFFormValue.h"
 #include "llvm/Support/Dwarf.h"
 #include "gtest/gtest.h"
 using namespace llvm;
 using namespace dwarf;
 
 namespace {
 
EXPECT_TRUE(
       isFormClass(DW_FORM_sec_offset, DWARFFormValue::FC_SectionOffset));
   EXPECT_TRUE(isFormClass(DW_FORM_GNU_str_index, DWARFFormValue::FC_String));
   EXPECT_TRUE(isFormClass(DW_FORM_GNU_addr_index, DWARFFormValue::FC_Address));
   EXPECT_FALSE(isFormClass(DW_FORM_ref_addr, DWARFFormValue::FC_Address));
   EXPECT_TRUE(isFormClass(DW_FORM_ref_addr, DWARFFormValue::FC_Reference));
   EXPECT_TRUE(isFormClass(DW_FORM_ref_sig8, DWARFFormValue::FC_Reference));
 }
 
+
+class DWARFTestContext : public DWARFContext {
+  Section DummySection;
+  TypeSectionMap DummySectionMap;
+public:
+  bool isLittleEndian() const override { return sys::IsLittleEndianHost; }
+  uint8_t getAddressSize() const override { return sizeof(void *); }
+  const Section &getInfoSection() override { return DummySection; }
+  const TypeSectionMap &getTypesSections() override { return DummySectionMap; }
+  StringRef getAbbrevSection() override { return StringRef(); }
+  const Section &getLocSection() override { return DummySection; }
+  const Section &getLocDWOSection() override { return DummySection; }
+  StringRef getARangeSection() override { return StringRef(); }
+  StringRef getDebugFrameSection() override { return StringRef(); }
+  const Section &getLineSection() override { return DummySection; }
+  const Section &getLineDWOSection() override { return DummySection; }
+  StringRef getStringSection() override { return StringRef(); }
+  StringRef getRangeSection() override { return StringRef(); }
+  StringRef getPubNamesSection() override { return StringRef(); }
+  StringRef getPubTypesSection() override { return StringRef(); }
+  StringRef getGnuPubNamesSection() override { return StringRef(); }
+  StringRef getGnuPubTypesSection() override { return StringRef(); }
+  const Section &getInfoDWOSection() override { return DummySection; }
+  const TypeSectionMap &getTypesDWOSections() override { return DummySectionMap; }
+  StringRef getAbbrevDWOSection() override { return StringRef(); }
+  StringRef getStringDWOSection() override { return StringRef(); }
+  StringRef getStringOffsetDWOSection() override { return StringRef(); }
+  StringRef getRangeDWOSection() override { return StringRef(); }
+  StringRef getAddrSection() override { return StringRef(); }
+};
+
+
+static DWARFTestContext TestContext;
+static DWARFUnitSection<DWARFCompileUnit> DummyUnitSection;
+static RelocAddrMap DummyRelocMap;
+static DWARFCompileUnit DummyCompileUnit(TestContext, nullptr, StringRef(),
+                                         StringRef(), StringRef(), StringRef(),
+                                         StringRef(), &DummyRelocMap,
+                                         sys::IsLittleEndianHost, DummyUnitSection);
+
+DWARFFormValue createFromValue(uint16_t Form, uint64_t Value) {
+  union {
+    uint64_t Val;
+    char     Raw[8];
+  } U = { Value };
+  uint32_t Offset = 0;
+  DWARFFormValue Result(Form);
+  DataExtractor Data(U.Raw, sys::IsLittleEndianHost, sizeof(void*));
+  Result.extractValue(Data, &Offset, &DummyCompileUnit);
+  return Result;
+}
+
+TEST(DWARFFormValue, SignedConstantForms) {
+  DWARFFormValue data1 = createFromValue(DW_FORM_data1, 0xFF);
+  DWARFFormValue data2 = createFromValue(DW_FORM_data2, 0xFFFF);
+  DWARFFormValue data4 = createFromValue(DW_FORM_data4, 0xFFFFFFFF);
+  DWARFFormValue data8 = createFromValue(DW_FORM_data8, 0xFFFFFFFFFFFFFFFF);
+  EXPECT_EQ(data1.getAsSignedConstant().getValue(), -1);
+  EXPECT_EQ(data2.getAsSignedConstant().getValue(), -1);
+  EXPECT_EQ(data4.getAsSignedConstant().getValue(), -1);
+  EXPECT_EQ(data8.getAsSignedConstant().getValue(), -1);
+}
+
 } // end anonymous namespace