[PATCH] D19975: [scan-build] fix warning emitted on LLVM Bitcode code base

[Teresa Johnson via llvm-commits]

On Thu, May 5, 2016 at 4:03 PM, Justin Bogner <mail at justinbogner.com> wrote:

> Apelete Seketeli via llvm-commits <llvm-commits at lists.llvm.org> writes:
> > apelete updated this revision to Diff 56361.
> > apelete added a comment.
> >
> > [scan-build] fix warning emitted on LLVM Bitcode code base
> >
> > Changes since last revision:
> >
> > - initialize FnEntry8BitAbbrev, FnEntry7BitAbbrev and
> > FnEntry6BitAbbrev variables to 0 and assert they are non-zero prior
> > being read.
> >
> >
> > http://reviews.llvm.org/D19975
> >
> > Files:
> >   lib/Bitcode/Writer/BitcodeWriter.cpp
> >
> > Index: lib/Bitcode/Writer/BitcodeWriter.cpp
> > ===================================================================
> > --- lib/Bitcode/Writer/BitcodeWriter.cpp
> > +++ lib/Bitcode/Writer/BitcodeWriter.cpp
> > @@ -2507,9 +2507,9 @@
> >
> >    // For the module-level VST, add abbrev Ids for the VST_CODE_FNENTRY
> >    // records, which are not used in the per-function VSTs.
> > -  unsigned FnEntry8BitAbbrev;
> > -  unsigned FnEntry7BitAbbrev;
> > -  unsigned FnEntry6BitAbbrev;
> > +  unsigned FnEntry8BitAbbrev = 0;
> > +  unsigned FnEntry7BitAbbrev = 0;
> > +  unsigned FnEntry6BitAbbrev = 0;
> >    if (IsModuleLevel && hasVSTOffsetPlaceholder()) {
> >      // 8-bit fixed-width VST_CODE_FNENTRY function strings.
> >      BitCodeAbbrev *Abbv = new BitCodeAbbrev();
> > @@ -2582,11 +2582,15 @@
> >        NameVals.push_back(BitcodeIndex / 32);
> >
> >        Code = bitc::VST_CODE_FNENTRY;
> > +      assert(FnEntry8BitAbbrev && "abbrev was not computed");
> >        AbbrevToUse = FnEntry8BitAbbrev;
> > -      if (Bits == SE_Char6)
> > +      if (Bits == SE_Char6) {
> > +        assert(FnEntry6BitAbbrev && "abbrev was not computed");
> >          AbbrevToUse = FnEntry6BitAbbrev;
> > -      else if (Bits == SE_Fixed7)
> > +      } else if (Bits == SE_Fixed7) {
> > +        assert(FnEntry7BitAbbrev && "abbrev was not computed");
> >          AbbrevToUse = FnEntry7BitAbbrev;
> > +      }
>
> This isn't a very maintainable approach. If other uses of these abbrevs
> pop up in different paths they would also need the asserts, and no
> compiler diagnostics or memory error tools would help to find them.
>

I tend to agree with Justin (and David Blaikie, who pointed this out in a
separate thread), that fixing things this way can actually hide real errors
from the other sanitizers. I'd like to see some resolution on that
other thread ("Before we go cleaning up LLVM+Clang of all Static Analyzer
Warnings...") before we decide to fix these this way.


>
> Case in point, there are two more uses of 6_ABBREV and 7_ABBREV a couple
> of lines below.
>

I actually don't think this is an issue - those abbrev ids are computed
unconditionally elsewhere. It is only the FnEntry*BitAbbrevs that are
conditionally setup here (for the module level VST only).

Teresa


> All in all, I think that fixing the sanitizer warning in this way is
> actually making this code more prone to hard to detect bugs.
>
> >      } else {
> >        Code = bitc::VST_CODE_ENTRY;
> >        if (Bits == SE_Char6)
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |  408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160505/4f09a739/attachment.html>