[DebugInfo][Support] Replace DWARFDataExtractor size function

[jhenderson]

Summary

This patch adds a new size function to the base DataExtractor class, which removes the need for the DWARFDataExtractor size function.

It is unclear why DWARFDataExtractor's size function returned zero in some circumstances (i.e. when it is constructed without a section, and with a different data source instead), so that behaviour has changed. The old behaviour could cause an assertion in the debug line parser, as the size did not reflect the actual data available, and could be lower than the current offset being parsed.

[dblaikie]

The old behaviour could cause an assertion in the debug line parser

Could you include a test case for this, demonstrating the assertion is no longer triggering?

[jhenderson]

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Also, adding the unit test makes this patch now depend on D72154 (well, not strictly - I could implement without the dependency, but the two overlap in my branch).

[dblaikie]

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Also, adding the unit test makes this patch now depend on D72154 (well, not strictly - I could implement without the dependency, but the two overlap in my branch).

[jhenderson]

In D72337#1813202, @dblaikie wrote:

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Oh, sorry for misunderstanding. We have a local test for a local patch in LLD that makes some use of the debug information, but this patch doesn't create the data extractor with a section, which meant that it ran into this assertion. I checked, and I can't find any other usage of this function with a data extractor constructed without a section.

[dblaikie]

In D72337#1814108, @jhenderson wrote:

In D72337#1813202, @dblaikie wrote:

In D72337#1811669, @jhenderson wrote:

Added unit test case that would trigger the assertion mentioned in the description. I'm not entirely convinced that this test case is worthwhile, since with the change in how size() is calculated, it really is moot (e.g. I wouldn't have needed it if the size() function had always been the way I'm proposing).

Ah, I was more expecting an end-to-end test (with llvm-dwarfdump or similar) when you said in the patch description "The old behaviour could cause an assertion in the debug line parser"

What were the circumstances where this assertion would fail? Was the failure reachable with llvm-dwarfdump, or only via a unit test/non-production API usage?

Oh, sorry for misunderstanding. We have a local test for a local patch in LLD that makes some use of the debug information, but this patch doesn't create the data extractor with a section, which meant that it ran into this assertion. I checked, and I can't find any other usage of this function with a data extractor constructed without a section.

Ah, thanks for the context! Yeah, go ahead and commit it without a test case - the new implementation is strictly simpler/more obvious & I don't think the unit test really adds value/is likely to catch any regressions here.

[jhenderson]

Rebased.

[jhenderson]

In D72337#1814124, @dblaikie wrote:

Ah, thanks for the context! Yeah, go ahead and commit it without a test case - the new implementation is strictly simpler/more obvious & I don't think the unit test really adds value/is likely to catch any regressions here.

Thanks. I didn't see your comment when I was doing the rebase, but I'll make sure to remove the extra unit test before landing it on Monday.

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

public:
   }
 
   /// Extracts a DWARF-encoded pointer in \p Offset using \p Encoding.
   /// There is a DWARF encoding that uses a PC-relative adjustment.
   /// For these values, \p AbsPosOffset is used to fix them, which should
   /// reflect the absolute address of this pointer.
   Optional<uint64_t> getEncodedPointer(uint64_t *Offset, uint8_t Encoding,
                                        uint64_t AbsPosOffset = 0) const;
-
-  size_t size() const { return Section == nullptr ? 0 : Section->Data.size(); }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDATAEXTRACTOR_H

llvm/include/llvm/Support/DataExtractor.h

public:
   /// Advance the Cursor position by the given number of bytes. No-op if the
   /// cursor is in an error state.
   void skip(Cursor &C, uint64_t Length) const;
 
   /// Return true iff the cursor is at the end of the buffer, regardless of the
   /// error state of the cursor. The only way both eof and error states can be
   /// true is if one attempts a read while the cursor is at the very end of the
   /// data buffer.
-  bool eof(const Cursor &C) const { return Data.size() == C.Offset; }
+  bool eof(const Cursor &C) const { return size() == C.Offset; }
 
   /// Test the validity of \a offset.
   ///
   /// @return
   ///     \b true if \a offset is a valid offset into the data in this
   ///     object, \b false otherwise.
-  bool isValidOffset(uint64_t offset) const { return Data.size() > offset; }
+  bool isValidOffset(uint64_t offset) const { return size() > offset; }
 
   /// Test the availability of \a length bytes of data from \a offset.
   ///
   /// @return
   ///     \b true if \a offset is a valid offset and there are \a
   ///     length bytes available at that offset, \b false otherwise.
   bool isValidOffsetForDataOfSize(uint64_t offset, uint64_t length) const {
     return offset + length >= offset && isValidOffset(offset + length - 1);
   }
 
   /// Test the availability of enough bytes of data for a pointer from
   /// \a offset. The size of a pointer is \a getAddressSize().
   ///
   /// @return
   ///     \b true if \a offset is a valid offset and there are enough
   ///     bytes for a pointer available at that offset, \b false
   ///     otherwise.
   bool isValidOffsetForAddress(uint64_t offset) const {
     return isValidOffsetForDataOfSize(offset, AddressSize);
   }
 
+  /// Return the number of bytes in the underlying buffer.
+  size_t size() const { return Data.size(); }
+
 protected:
   // Make it possible for subclasses to access these fields without making them
   // public.
   static uint64_t &getOffset(Cursor &C) { return C.Offset; }
   static Error &getError(Cursor &C) { return C.Err; }
 };
 
 } // namespace llvm
 
 #endif

llvm/unittests/Support/DataExtractorTest.cpp

TEST(DataExtractorTest, eof) {
   EXPECT_EQ(0, DE.getU16(C));
   EXPECT_FALSE(DE.eof(C));
   EXPECT_THAT_ERROR(C.takeError(), Failed());
 
   EXPECT_EQ('A', DE.getU8(C));
   EXPECT_TRUE(DE.eof(C));
   EXPECT_THAT_ERROR(C.takeError(), Succeeded());
 }
+
+TEST(DataExtractorTest, size) {
+  uint8_t Data[] = {'A', 'B', 'C', 'D'};
+  DataExtractor DE1(StringRef(reinterpret_cast<char *>(Data), sizeof(Data)),
+                    false, 8);
+  EXPECT_EQ(DE1.size(), sizeof(Data));
+  DataExtractor DE2(ArrayRef<uint8_t>(Data), false, 8);
+  EXPECT_EQ(DE2.size(), sizeof(Data));
+}
 }