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

[Alexander Kornienko]

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



-- 
Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120723/87898b36/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cl-opt-init-test.diff
Type: application/octet-stream
Size: 917 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120723/87898b36/attachment.obj>