[PATCH] D74413: [DebugInfo] Add checks for v2 directory and file name table terminators
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 12 04:41:49 PST 2020
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
More information about the llvm-commits
mailing list