[PATCH] D33102: [clang] Implement -Wcast-qual for C++

[Roman Lebedev via cfe-commits]

On Mon, Jun 12, 2017 at 8:16 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Jun 12, 2017 at 10:10 AM Roman Lebedev via Phabricator
> <reviews at reviews.llvm.org> wrote:
>>
>> lebedev.ri added a comment.
>>
>> So i'm trying to analyze that stage2 warning.
>
>
> Could you link to the buildbot failure to see the original LLVM project code
> triggering this situation?
http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/3249
http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/3249/steps/build-stage2-LLVMgold.so/logs/stdio

FAILED: /home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/install/stage1/bin/clang++
  -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_GLOBAL_ISEL -D_DEBUG -D_GNU_SOURCE
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
-Ilib/CodeGen/AsmPrinter
-I/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter
-Iinclude -I/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/include
-fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -std=c++11
-Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual
-Wmissing-field-initializers -pedantic -Wno-long-long
-Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor
-Wstring-conversion -fcolor-diagnostics -ffunction-sections
-fdata-sections -O3    -UNDEBUG  -fno-exceptions -fno-rtti -MMD -MT
lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/CodeViewDebug.cpp.o
-MF lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/CodeViewDebug.cpp.o.d
-o lib/CodeGen/AsmPrinter/CMakeFiles/LLVMAsmPrinter.dir/CodeViewDebug.cpp.o
-c /home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
In file included from
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:14:
In file included from
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter/CodeViewDebug.h:17:
In file included from
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h:15:
In file included from
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/include/llvm/IR/DebugInfoMetadata.h:26:
In file included from
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/include/llvm/IR/Metadata.h:23:
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/include/llvm/ADT/PointerUnion.h:161:19:
error: cast from 'void **' to 'const llvm::DISubprogram **' must have
all intermediate pointers const qualified to be safe
[-Werror,-Wcast-qual]
    return (PT1 *)Val.getAddrOfPointer();
                  ^
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/include/llvm/ADT/TinyPtrVector.h:177:18:
note: in instantiation of member function 'llvm::PointerUnion<const
llvm::DISubprogram *, llvm::SmallVector<const llvm::DISubprogram *, 4>
*>::getAddrOfPtr1' requested here
      return Val.getAddrOfPtr1();
                 ^
/home/buildbot/Buildbot/Slave1a/clang-with-lto-ubuntu/llvm.src/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1885:33:
note: in instantiation of member function 'llvm::TinyPtrVector<const
llvm::DISubprogram *>::begin' requested here
    for (const DISubprogram *SP : MethodItr.second) {
                                ^


>>
>> The testcase //seems// to be: (autogenerated all the variants)
>>
>>   void test_nop() {
>>     unsigned char **ptr1 = 0;
>>     void **ptr2 = (void **)ptr1;
>>   }
>>   void test_bad() {
>>     unsigned char **ptr1 = 0;
>>     const void **ptr2 = (const void **)ptr1; // expected-warning {{cast
>> from 'unsigned char **' to 'const void **' must have all intermediate
>> pointers const qualified to be safe}}
>>   }
>>   void test_good0() {
>>     unsigned char **ptr1 = 0;
>>     void *const *ptr2 = (void *const *)ptr1;
>>   }
>>   void test_good1() {
>>     unsigned char **ptr1 = 0;
>>     const void *const *ptr2 = (const void *const *)ptr1;
>>   }
>>   void test_good2() {
>>     unsigned char *const *ptr1 = 0;
>>     void *const *ptr2 = (void *const *)ptr1;
>>   }
>>   void test_good3() {
>>     unsigned char *const *ptr1 = 0;
>>     const void *const *ptr2 = (const void *const *)ptr1;
>>   }
>>   void test_good4() {
>>     const unsigned char **ptr1 = 0;
>>     const void **ptr2 = (const void **)ptr1;
>>   }
>>   void test_good5() {
>>     const unsigned char **ptr1 = 0;
>>     const void *const *ptr2 = (const void *const *)ptr1;
>>   }
>>   void test_good6() {
>>     const unsigned char *const *ptr1 = 0;
>>     const void *const *ptr2 = (const void *const *)ptr1;
>>   }
>>
>> GCC does not warn about such code at all, clang in C mode does warn about
>> only one combination:
>>
>>   $ gcc -c /tmp/test.c -Wcast-qual
>>   $ echo $?
>>   0
>>   $ clang -c /tmp/test.c -Wcast-qual
>>   /tmp/test.c:7:38: warning: cast from 'unsigned char **' to 'const void
>> **' must have all intermediate pointers const qualified to be safe
>> [-Wcast-qual]
>>     const void **ptr2 = (const void **)ptr1; // expected-warning {{cast
>> from 'unsigned char **' to 'const void **' must have all intermediate
>> pointers const qualified to be safe}}
>>                                        ^
>>   1 warning generated.
>>
>> David, you reviewed the original `-Wcast-qual` patch, does all that ^ make
>> sense?
>
>
> That seems like a reasonable warning, do you agree?
I think all that makes sense. I just want to have a second opinion,
especially about rest of these cases where it does not fire.

> But maybe it's best to put it under another flag name so it doesn't collide
> with GCC.

> But really - *shrug* I'd probably leave it under the same flag, fix the LLVM
> project code that causes it, and carry on.
I hope that will be the solution.

>>
>>
>> F3429854: gen.cpp <https://reviews.llvm.org/F3429854>
>>
>>
>> Repository:
>>   rL LLVM
>>
>> https://reviews.llvm.org/D33102
>>
>>
>>
>