[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 15:21:44 PDT 2012


On Mon, Jun 18, 2012 at 9:19 AM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Jun 18, 2012 at 8:37 AM, Duncan Sands <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.

I believe it's this:

diff --git include/llvm/ADT/DenseMap.h include/llvm/ADT/DenseMap.h
index 045b5c6..0166228 100644
--- include/llvm/ADT/DenseMap.h
+++ include/llvm/ADT/DenseMap.h
@@ -490,7 +490,7 @@ private:

   template <typename LookupKeyT>
   bool LookupBucketFor(const LookupKeyT &Val, BucketT *&FoundBucket) {
-    const BucketT *ConstFoundBucket = FoundBucket;
+    const BucketT *ConstFoundBucket;
     bool Result = const_cast<const DenseMapBase *>(this)
       ->LookupBucketFor(Val, ConstFoundBucket);
     FoundBucket = const_cast<BucketT *>(ConstFoundBucket);

When called with an uninitialized variable for 'FoundBucket' this code
invokes UB by attempting to copy that uninitialized variable. The
underlying function (the const version of LookupBucketFor) doesn't use
the value anyway, so simply removing the initialization should be
enough to tidy it up.

[side note: do we have an "implicit_cast<T>" you could use there
instead of the const_cast - I generally only expect to see const_casts
/away/ from const (& thus treat them with appropriate care/concern) -
not /to/ const]

- David

> 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.
> -Chandler
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



More information about the llvm-commits mailing list