[llvm] r185227 - Fix broken asserts that never fire.

[Richard Trieu]

On Fri, Jun 28, 2013 at 3:46 PM, Bill Wendling <wendling at apple.com> wrote:

>
> On Jun 28, 2013, at 3:39 PM, Richard Trieu <rtrieu at google.com> wrote:
>
> >
> >
> >
> > On Fri, Jun 28, 2013 at 3:23 PM, Bill Wendling <wendling at apple.com>
> wrote:
> >
> > On Jun 28, 2013, at 3:20 PM, Richard Trieu <rtrieu at google.com> wrote:
> >
> > > This time, reply to the mailing list as well.
> > >
> > > On Fri, Jun 28, 2013 at 3:00 PM, Bill Wendling <wendling at apple.com>
> wrote:
> > >
> > > On Jun 28, 2013, at 2:54 PM, Richard Trieu <rtrieu at google.com> wrote:
> > >
> > > > Author: rtrieu
> > > > Date: Fri Jun 28 16:54:25 2013
> > > > New Revision: 185227
> > > >
> > > > URL: http://llvm.org/viewvc/llvm-project?rev=185227&view=rev
> > > > Log:
> > > > Fix broken asserts that never fire.
> > > >
> > > > Change assert("text") to assert(0 && "text").  The first case is a
> const char *
> > > > to bool conversion, which always evaluates to true, never triggering
> the
> > > > assert.  The second case will always trigger the assert.
> > > >
> > > > Modified:
> > > >    llvm/trunk/include/llvm/TableGen/Record.h
> > > >    llvm/trunk/lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp
> > > >
> > > > Modified: llvm/trunk/include/llvm/TableGen/Record.h
> > > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/TableGen/Record.h?rev=185227&r1=185226&r2=185227&view=diff
> > > >
> ==============================================================================
> > > > --- llvm/trunk/include/llvm/TableGen/Record.h (original)
> > > > +++ llvm/trunk/include/llvm/TableGen/Record.h Fri Jun 28 16:54:25
> 2013
> > > > @@ -1799,9 +1799,9 @@ struct LessRecordRegister {
> > > >
> > > >       unsigned LHSVal, RHSVal;
> > > >       if (LHSPart.second.getAsInteger(10, LHSVal))
> > > > -        assert("Unable to convert LHS to integer.");
> > > > +        assert(0 && "Unable to convert LHS to integer.");
> > > >       if (RHSPart.second.getAsInteger(10, RHSVal))
> > > > -        assert("Unable to convert RHS to integer.");
> > > > +        assert(0 && "Unable to convert RHS to integer.");
> > >
> > > Why do it like this?
> > >
> > >      assert(LHSPart.second.getAsInteger(10, LHSVal) && "Unable to
> convert LHS to integer.");
> > >      assert(RHSPart.second.getAsInteger(10, RHSVal) &&"Unable to
> convert RHS to integer.");
> > >
> > > -bw
> > >
> > > On non-assert builds, getAsInteger wouldn't run, so LHSVal and RHSVal,
> which is used later, will not get computed.
> >
> >
> > Then do this. :-)
> >
> >     LHSPart.second.getAsInteger(10, LHSVal);
> >     RHSPart.second.getAsInteger(10, LHSVal)
> >     assert(LHSVal && "Unable to convert LHS to integer.");
> >     assert(RHSVal && "Unable to convert RHS to integer.");
> >
> > -bw
> >
> > LHSVal and RHSVal are type unsigned and uninitialized when getAsInteger
> is called.  When getAsInteger fails, LHSVal and RHSVal will remain
> uninitialized.  So it is possible that the assert condition will be
> evaluated on uninitialized values.
>
> Hmm...if only there were an easy solution to that. ;-)
>
> -bw
>
> I'm a bit hesitant to go changing code I'm not that familiar with.  It
seems like you are suggesting to zero initialize the variables, then check
that they are unchanged.  However, that assumes that getAsInteger will
never set the variable to 0.


> > > >       if (LHSVal != RHSVal)
> > > >         return LHSVal < RHSVal;
> > > >     }
> > > >
> > > > Modified:
> llvm/trunk/lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp
> > > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp?rev=185227&r1=185226&r2=185227&view=diff
> > > >
> ==============================================================================
> > > > --- llvm/trunk/lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp
> (original)
> > > > +++ llvm/trunk/lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp
> Fri Jun 28 16:54:25 2013
> > > > @@ -179,7 +179,7 @@ void HexagonInstPrinter::printBranchOper
> > > >                                             raw_ostream &O) const {
> > > >   // Branches can take an immediate operand.  This is used by the
> branch
> > > >   // selection pass to print $+8, an eight byte displacement from
> the PC.
> > > > -  assert("Unknown branch operand.");
> > > > +  assert(0 && "Unknown branch operand.");
> > > > }
> > > >
> > > > void HexagonInstPrinter::printCallOperand(const MCInst *MI, unsigned
> OpNo,
> > > > @@ -203,7 +203,7 @@ void HexagonInstPrinter::printSymbol(con
> > > >     O << '#';
> > > >     printOperand(MI, OpNo, O);
> > > >   } else {
> > > > -    assert("Unknown symbol operand");
> > > > +    assert(0 && "Unknown symbol operand");
> > > >     printOperand(MI, OpNo, O);
> > >
> > > The 'printOperand' instruction is now dead.
> > >
> > > I'd prefer someone who knows this code to make that change.  I'm only
> here to fix the string literal to bool conversions.
> > >
> > > >   }
> > > >   O << ')';
> > > >
> > > >
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at cs.uiuc.edu
> > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > >
> > >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130628/47941bbf/attachment.html>