[llvm-commits] [llvm] r160170 - /llvm/trunk/include/llvm/Support/CommandLine.h

[Duncan Sands]

Hi Alexander,

> On Fri, Jul 13, 2012 at 3:06 PM, Duncan Sands <baldrick at free.fr
> <mailto:baldrick at free.fr>> wrote:
>
>     Hi Alexander,
>      > Initializers for some fields were missing in Option::Option
>
>     did it matter?  If so, please add a testcase.  If not, why this change?
>
> This did matter. It caused random crashes in case cl::opt was a member of
> non-POD class. And I always considered it obvious that uninitialized data is
> bad. Don't you think so?

no, I don't think so, because initializing variables can hide bugs.  For
example, if a variable is not supposed to be used in some context but is,
then initializing it to some random value (eg 0) doesn't make the code
correct, as it is still computing a bogus result, but it does mean that
tools like valgrind won't find the bug any more.

> The change which lead me to discovery of this bug is here:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120716/060757.html,
> diff here:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20120716/6f1649d3/attachment-0001.obj
>
> As for a test case, I'm not sure I can come up with a test case which relies on
> uninitialized data. If you can suggest something, I will certainly implement one.

All you need is a testcase where the use of the unitialized data would be
picked up by valgrind, since some builders run the tests under valgrind.
I don't know if the unittest suite has a way to say "run this test under
valgrind if available", perhaps Chandler knows.

Ciao, Duncan.

>
>
>     Ciao, Duncan.
>
>      >
>      > Modified:
>      >      llvm/trunk/include/llvm/Support/CommandLine.h
>      >
>      > Modified: llvm/trunk/include/llvm/Support/CommandLine.h
>      > URL:
>     http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=160170&r1=160169&r2=160170&view=diff
>      >
>     ==============================================================================
>      > --- llvm/trunk/include/llvm/Support/CommandLine.h (original)
>      > +++ llvm/trunk/include/llvm/Support/CommandLine.h Fri Jul 13 07:55:23 2012
>      > @@ -217,11 +217,11 @@
>      >     void setMiscFlag(enum MiscFlags M) { Misc |= M; }
>      >     void setPosition(unsigned pos) { Position = pos; }
>      >   protected:
>      > -  explicit Option(enum NumOccurrencesFlag OccurrencesFlag,
>      > +  explicit Option(enum NumOccurrencesFlag OccurrencesFlag,
>      >                     enum OptionHidden Hidden)
>      > -    : NumOccurrences(0), Occurrences(OccurrencesFlag), HiddenFlag(Hidden),
>      > -      Formatting(NormalFormatting), Position(0),
>      > -      AdditionalVals(0), NextRegistered(0),
>      > +    : NumOccurrences(0), Occurrences(OccurrencesFlag), Value(0),
>      > +      HiddenFlag(Hidden), Formatting(NormalFormatting), Misc(0),
>      > +      Position(0), AdditionalVals(0), NextRegistered(0),
>      >         ArgStr(""), HelpStr(""), ValueStr("") {
>      >     }
>      >
>      >
>      >
>      > _______________________________________________
>      > llvm-commits mailing list
>      > llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>      > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>      >
>
>
> --
> Regards,
> Alexander