[PATCH] D74413: [DebugInfo] Add checks for v2 directory and file name table terminators

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 5 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:165-170
   while (*OffsetPtr < EndPrologueOffset) {
     StringRef S = DebugLineData.getCStrRef(OffsetPtr);
-    if (S.empty())
+    if (S.empty()) {
+      IncludeDirsTerminated = true;
       break;
+    }
----------------
MaskRay wrote:
> dblaikie wrote:
> > Perhaps this'd be a little more terse (but not necessarily better) written as:
> > 
> > ```
> > Terminated = S.empty();
> > if (Terminated)
> >   break;
> > ```
> > or maybe
> > ```
> > if ((Terminated = S.empty()))
> >   break;
> > ```
> > 
> > Not necessary if you prefer the current version.
> > 
> > I'd probably more strongly suggest making the variable names a bit simpler - probably just using "Terminated" for both.
> Terminated +1
> 
> The variable `IncludeDirectories` below suggests what `Terminated` means.
> 
> Both the current form and 
> 
>   if ((Terminated = S.empty()))
>     break;
> 
> look fine to me.
I think I marginally prefer the current form for readability reasons, so I'll leave that as is. The renaming makese sense though. I considered making that change before putting it up for review.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:178
+    return createStringError(errc::invalid_argument,
+                             "include directories table was not terminated "
+                             "before the end of the prologue");
----------------
dblaikie wrote:
> maybe make it a bit more explicit by saying "null terminated"?
That's fine with me, although I should point out that the strings themselves are always null terminated, so even if the null terminator byte is missing, the table will still have a null byte at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74413