[llvm-commits] [llvm] r158638 - in /llvm/trunk: include/llvm/ADT/DenseMap.h unittests/ADT/DenseMapTest.cpp

David Blaikie dblaikie at gmail.com
Mon Jun 18 21:20:24 PDT 2012


On Mon, Jun 18, 2012 at 9:12 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
> Chandler Carruth wrote:
>> On Mon, Jun 18, 2012 at 8:37 AM, Duncan Sands <baldrick at free.fr
>> <mailto:baldrick at free.fr>> wrote:
>>
>>     Hi Chandler,
>>
>>      > The codebase already has piles of these incorrect warnings in it,
>>
>>     maybe clang does but LLVM proper does not, so why not keep it that way?
>>
>>
>> Because the warning is broken. Fundamentally.
>>
>> There is no way to work around the warning that does not make the code
>> worse. If the GCC optimizer cannot *prove* that the value is
>> initialized, or does not hit one of its magical "don't warn on this"
>> signals (usually escaped address to an external function), it calls it
>> uninitialized.
>>
>> If you initialize it blindly, then any actual bug you might have in your
>> code will fail to be reported by Valgrind and other tools that really
>> can detect uninitialized uses.
>>
>> Now, maybe I've introduced a bug into this code and the warning is
>> legitimate. I'll look into that. But I am completely opposed to changing
>> code to be less readable, less accurate, less efficient, or less easy
>> for Valgrind to check to satisfy a poorly implemented warning when we
>> have better alternatives.
>>
>> We should rely on Clang's -Wuninitialized and on Valgrind to catch our bugs.
>
> I agree with this in this case, but it hasn't escaped my attention that
> LLVM has always had a "no warnings allowed" policy which we've applied
> evenly across all situations (see
> http://llvm.org/docs/DeveloperPolicy.html#quality ). Since I think this
> is a legitimate problem, I want to discuss what the policy ought to be.
>
> We should err heavily towards the side of respecting the compilers
> warning, except when we know that the compiler warning is Plain Broken
> (as it is here).

It seems it's added some value - with a relatively small handful of
false positives (that, admittedly, don't seem to have a workaround -
though perhaps self-initialization silences the warning while still
preserving Valgrind's ability to catch problems?) that linger in the
codebase. I'm not entirely ready to dismiss this warning completely.

> We should have a list of warning+compiler pairs that
> are known broken (perhaps next to the brokengcc list?).
>
> The reason that the old policy doesn't work any more is that as we've
> grown we've picked up more users using more distinct versions of more
> compilers than ever before, and our probability of hitting a badly
> implemented warning (even one that's just-a-bug from the compiler
> vendor) is going up.
>
> If folks agree with this, we should probably prepare a patch to the
> DeveloperPolicy to update its text slightly, and start our list somewhere.

Agreed - and/or enshrine these in the build system and buildbots
(perhaps have both the cmake and configure build generation default to
-Werror with an option to switch that off, rather than the other way
around?).

- David




More information about the llvm-commits mailing list