[PATCH] D72337: [DebugInfo][Support] Replace DWARFDataExtractor size function

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D72337#1814108 <https://reviews.llvm.org/D72337#1814108>, @jhenderson wrote:

> In D72337#1813202 <https://reviews.llvm.org/D72337#1813202>, @dblaikie wrote:
>
> > In D72337#1811669 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72337/new/

https://reviews.llvm.org/D72337