[PATCH] D37511: [dwarfdump] Verify line table prologue

[Greg Clayton via llvm-commits]

Gotcha. Makes sense.

> On Sep 6, 2017, at 11:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> The claim is that the index is valid by construction, not because of the input.
> 
> This seems to be true - the index comes from a loop over the Prologue.FileNames (offset by 1) & that's the only condition under which getFileNameByIndex returns false. (well, also Kind == None, and that's hardcoded at this callsite)
> 
> So I believe there's no possible input file that could cause this function to return 'false', so there's no error path here. (there would be no test that could be written to exercise it, the code would be dead)
> 
> On Wed, Sep 6, 2017 at 11:10 AM Greg Clayton via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> clayborg added a comment.
> 
> Why assert and not emit an error???
> 
> 
> 
> ================
> Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:455
> +      } else {
> +        llvm_unreachable("Invalid index?");
> +      }
> ----------------
> JDevlieghere wrote:
> > JDevlieghere wrote:
> > > dblaikie wrote:
> > > > Don't branch-to-unreachable. Use an assert instead.
> > > >
> > > > Is the false return from getFileNameByIndex really impossible? (has the FileIndex been checked for validity? What other error paths does getFileNameByIndex have, if any?)
> > > Thanks, an assert definitely better expresses the intent. The only path that returns false is this:
> > > ```
> > >   if (Kind == FileLineInfoKind::None || !hasFileAtIndex(FileIndex))
> > >     return false;
> > > ```
> > > The `FileLineInfoKind` is hard-coded so that can't trigger it. The index can't really be invalid either as we're iterating over the list of indices. If this returns false here it must be a programmer error.
> > What about the unused variable though? I need the function call to populate the string, so I'd have to assign it to a bool that's only used in the assert.
> This is a verifier that is trying to validate that the DWARF is valid. A bad file index is quite possible. Why not emit an appropriate error message here?
> 
> 
> Repository:
>   rL LLVM
> 
> https://reviews.llvm.org/D37511 <https://reviews.llvm.org/D37511>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170906/92c48f61/attachment.html>