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

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 12 11:50:46 PDT 2017 

Yeah, looks like the UB is baked in pretty deep here, so it's not
reasonable to try to fix it just because of this.

I'd probably suggest trying making that cast in PointerUnion.h into a
reinterpret cast? Would that suffice to address the const issues? Otherwise
a const_cast + reinterpret_cast?



On Mon, Jun 12, 2017 at 10:35 AM Roman Lebedev <lebedev.ri at gmail.com> wrote:

> 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
> >>
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170612/85d09ea2/attachment.html>

More information about the cfe-commits mailing list