Committed as r161751.<br><br><div class="gmail_quote">On Mon, Aug 13, 2012 at 10:25 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
LGTM. Ciao, Duncan.<div><div class="h5"><br>
<br>
On 23/07/12 19:52, Alexander Kornienko wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
I've added a test that shows a condition when my patch helps.<br>
It fails under valgrind with rev 160170 reverted:<br>
<br>
$ svn merge -r 160170:160169 <a href="https://llvm.org/svn/llvm-project/llvm/trunk" target="_blank">https://llvm.org/svn/llvm-<u></u>project/llvm/trunk</a><br>
$ cd your-build-dir<br>
$ make check-all<br>
$ valgrind unittests/Support/SupportTests<br>
...<br>
[ RUN ] CommandLineTest.<u></u>ParseEnvironmentToLocalVar<br>
==12673== Conditional jump or move depends on uninitialised value(s)<br>
==12673== at 0x659A17:<br>
GetOptionInfo(llvm::<u></u>SmallVectorImpl<llvm::cl::<u></u>Option*>&,<br>
llvm::SmallVectorImpl<llvm::<u></u>cl::Option*>&,<br>
llvm::StringMap<llvm::cl::<u></u>Option*, llvm::MallocAllocator>&) (in<br>
/usr/local/google/users/<u></u>alexfh/cmake-clang-build/<u></u>unittests/Support/<u></u>SupportTests)<br>
==12673== by 0x657E27: llvm::cl::<u></u>ParseCommandLineOptions(int, char const*<br>
const*, char const*, bool) (in<br>
/usr/local/google/users/<u></u>alexfh/cmake-clang-build/<u></u>unittests/Support/<u></u>SupportTests)<br>
==12673== by 0x657B70: llvm::cl::<u></u>ParseEnvironmentOptions(char const*,<br>
char const*, char const*, bool) (in<br>
/usr/local/google/users/<u></u>alexfh/cmake-clang-build/<u></u>unittests/Support/<u></u>SupportTests)<br>
==12673== by 0x495B2F: (anonymous<br>
namespace)::CommandLineTest_<u></u>ParseEnvironmentToLocalVar_<u></u>Test::TestBody() (in<br>
/usr/local/google/users/<u></u>alexfh/cmake-clang-build/<u></u>unittests/Support/<u></u>SupportTests)<br>
...<br>
<br>
With rev 160170 in place this valgrind message isn't triggered:<br>
<br>
$ svn revert include/llvm/Support/<u></u>CommandLine.h<br>
$ cd your-build-dir<br>
$ make check-all<br>
$ valgrind unittests/Support/SupportTests<br>
...<br>
[ RUN ] CommandLineTest.<u></u>ParseEnvironmentToLocalVar<br>
[ OK ] CommandLineTest.<u></u>ParseEnvironmentToLocalVar (10 ms)<br>
...<br>
<br>
There are also a few tests in tools/clang/tests/Tooling that fail on their own<br>
(i.e. without valgrind) without rev 160170, but these failures highly depend on<br>
external conditions and surrounding code layout (however, valgrind consistently<br>
finds issues even if the code doesn't break).<br>
<br>
Is it enough to prove validity of my previous patch?<br>
<br>
<br>
On Mon, Jul 16, 2012 at 3:28 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a><br></div></div><div class="im">
<mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>>> wrote:<br>
<br>
On Mon, Jul 16, 2012 at 6:24 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a><br></div><div class="im">
<mailto:<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>>> wrote:<br>
<br>
On Mon, Jul 16, 2012 at 3:10 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a><br></div><div class="im">
<mailto:<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>>> wrote:<br>
<br>
On Mon, Jul 16, 2012 at 6:02 AM, Alexander Kornienko<br></div><div class="im">
<<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a> <mailto:<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>>> wrote:<br>
<br>
<br>
On Fri, Jul 13, 2012 at 3:06 PM, Duncan Sands <<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a><br></div><div class="im">
<mailto:<a href="mailto:baldrick@free.fr" target="_blank">baldrick@free.fr</a>>> wrote:<br>
<br>
Hi Alexander,<br>
> Initializers for some fields were missing in Option::Option<br>
<br>
did it matter? If so, please add a testcase. If not, why<br>
this change?<br>
<br>
This did matter. It caused random crashes in case cl::opt was a<br>
member of non-POD class.<br>
<br>
<br>
Hold on, a *member*? 'cl::opt' objects are supposed to be globals,<br>
not members.<br>
<br>
We need it as a member to make a class which handles a common subset of<br>
command-line options for several clang tools. In a close future we'll<br>
need a control on which options should be enabled and which not. So we<br>
can't do it with global variables. Why can't we use it as a member?<br>
<br>
<br>
Errr... Ok, this is something we should discuss in more depth.<br>
<br>
Essentially, it's not that you necessarily can't, it's that the library was<br>
never designed with that use case in mind. It's completely outside the<br>
contract of the library, and so random things are likely to continue<br>
breaking. If this the correct design, we need to at least extend the library<br>
with unittests to check its behavior in that context. These unittests would<br>
then be what you would run under Valgrind in order to detect the failure.<br>
<br>
<br>
<br>
<br>
--<br>
Alexander Kornienko | Software Engineer |<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a><br></div>
<mailto:<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> |<a href="tel:%2B49%20151%20221%2077%20957" value="+4915122177957" target="_blank">+49 151 221 77 957</a><div class="im"><br>
Google Germany GmbH | Dienerstr. 12 | 80331 München<br>
<br>
</div></blockquote>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Best regards,</div><div>Alexander Kornienko</div><br>