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

[Alexander Kornienko]

Committed as r161751.

On Mon, Aug 13, 2012 at 10:25 AM, Duncan Sands <baldrick at free.fr> wrote:

> 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 <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
>>
>>
>


-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120813/513c1b3d/attachment.html>