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

[Duncan Sands]

LGTM.  Ciao, Duncan.

On 23/07/12 19:52, Alexander Kornienko wrote:
> I've added a test that shows a condition when my patch helps.
> It fails under valgrind with rev 160170 reverted:
>
>     $ svn merge -r 160170:160169 https://llvm.org/svn/llvm-project/llvm/trunk
>     $ cd your-build-dir
>     $ make check-all
>     $ valgrind unittests/Support/SupportTests
>     ...
>     [ RUN      ] CommandLineTest.ParseEnvironmentToLocalVar
>     ==12673== Conditional jump or move depends on uninitialised value(s)
>     ==12673==    at 0x659A17:
>     GetOptionInfo(llvm::SmallVectorImpl<llvm::cl::Option*>&,
>     llvm::SmallVectorImpl<llvm::cl::Option*>&,
>     llvm::StringMap<llvm::cl::Option*, llvm::MallocAllocator>&) (in
>     /usr/local/google/users/alexfh/cmake-clang-build/unittests/Support/SupportTests)
>     ==12673==    by 0x657E27: llvm::cl::ParseCommandLineOptions(int, char const*
>     const*, char const*, bool) (in
>     /usr/local/google/users/alexfh/cmake-clang-build/unittests/Support/SupportTests)
>     ==12673==    by 0x657B70: llvm::cl::ParseEnvironmentOptions(char const*,
>     char const*, char const*, bool) (in
>     /usr/local/google/users/alexfh/cmake-clang-build/unittests/Support/SupportTests)
>     ==12673==    by 0x495B2F: (anonymous
>     namespace)::CommandLineTest_ParseEnvironmentToLocalVar_Test::TestBody() (in
>     /usr/local/google/users/alexfh/cmake-clang-build/unittests/Support/SupportTests)
>     ...
>
> With rev 160170 in place this valgrind message isn't triggered:
>
>     $ svn revert include/llvm/Support/CommandLine.h
>     $ cd your-build-dir
>     $ make check-all
>     $ valgrind unittests/Support/SupportTests
>     ...
>     [ RUN      ] CommandLineTest.ParseEnvironmentToLocalVar
>     [       OK ] CommandLineTest.ParseEnvironmentToLocalVar (10 ms)
>     ...
>
> There are also a few tests in tools/clang/tests/Tooling that fail on their own
> (i.e. without valgrind) without rev 160170, but these failures highly depend on
> external conditions and surrounding code layout (however, valgrind consistently
> finds issues even if the code doesn't break).
>
> Is it enough to prove validity of my previous patch?
>
>
> On Mon, Jul 16, 2012 at 3:28 PM, Chandler Carruth <chandlerc at google.com
> <mailto:chandlerc at google.com>> wrote:
>
>     On Mon, Jul 16, 2012 at 6:24 AM, Alexander Kornienko <alexfh at google.com
>     <mailto:alexfh at google.com>> wrote:
>
>         On Mon, Jul 16, 2012 at 3:10 PM, Chandler Carruth <chandlerc at google.com
>         <mailto:chandlerc at google.com>> wrote:
>
>             On Mon, Jul 16, 2012 at 6:02 AM, Alexander Kornienko
>             <alexfh at google.com <mailto:alexfh at google.com>> wrote:
>
>
>                 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.
>
>
>             Hold on, a *member*? 'cl::opt' objects are supposed to be globals,
>             not members.
>
>         We need it as a member to make a class which handles a common subset of
>         command-line options for several clang tools. In a close future we'll
>         need a control on which options should be enabled and which not. So we
>         can't do it with global variables. Why can't we use it as a member?
>
>
>     Errr... Ok, this is something we should discuss in more depth.
>
>     Essentially, it's not that you necessarily can't, it's that the library was
>     never designed with that use case in mind. It's completely outside the
>     contract of the library, and so random things are likely to continue
>     breaking. If this the correct design, we need to at least extend the library
>     with unittests to check its behavior in that context. These unittests would
>     then be what you would run under Valgrind in order to detect the failure.
>
>
>
>
> --
> Alexander Kornienko | Software Engineer |alexfh at google.com
> <mailto:alexfh at google.com> |+49 151 221 77 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>