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

[Alexander Kornienko]

Ping.

On Mon, Jul 30, 2012 at 2:29 PM, Alexander Kornienko <alexfh at google.com>wrote:

> Ping.
>
>
> On Mon, Jul 23, 2012 at 7:52 PM, Alexander Kornienko <alexfh at google.com>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>wrote:
>>
>>> On Mon, Jul 16, 2012 at 6:24 AM, Alexander Kornienko <alexfh at google.com>wrote:
>>>
>>>> On Mon, Jul 16, 2012 at 3:10 PM, Chandler Carruth <chandlerc at google.com
>>>> > wrote:
>>>>
>>>>> On Mon, Jul 16, 2012 at 6:02 AM, Alexander Kornienko <
>>>>> alexfh at google.com> wrote:
>>>>>
>>>>>>
>>>>>> On Fri, Jul 13, 2012 at 3:06 PM, Duncan Sands <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.
>>>
>>
>>
>>

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