bjope added inline comments.
================ Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 + // FIXME check that the enum is in range. + auto Code = static_cast<llvm::bitc::FixedAbbrevIDs>(MaybeCode.get()); + ---------------- jfb wrote: > jfb wrote: > > bjope wrote: > > > This has caused problem for our sanitizer tests the last couple of days: > > > > > > ``` > > > FAIL: Extra Tools Unit Tests :: > > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of > > > 48746) > > > ******************** TEST 'Extra Tools Unit Tests :: > > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED > > > ******************** > > > Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode > > > [==========] Running 1 test from 1 test case. > > > [----------] Global test environment set-up. > > > [----------] 1 test from BitcodeTest > > > [ RUN ] BitcodeTest.emitMethodInfoBitcode > > > /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13: > > > runtime error: load of value 9, which is not a valid value for type > > > 'llvm::bitc::FixedAbbrevIDs' > > > > > > ``` > > > > > > This was seen when building trunk with clang 6.0.0 and > > > LVM_USE_SANITIZER=Undefined > > > > > > ``` > > > cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' > > > -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON > > > '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang > > > -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold > > > -DLLVM_USE_SANITIZER=Undefined ../. > > > -- The C compiler identification is Clang 6.0.0 > > > -- The CXX compiler identification is Clang 6.0.0 > > > ``` > > > > > > Afaict we can't cast the read value to FixedAbbrevIDs as we do not know > > > yet if it matches one of the values defined in the enum, or if we will > > > take the default case. > > > > > > > > > A similar switch exist at > > > cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is > > > using a slightly different pattern: > > > > > > ``` > > > unsigned Code; > > > Code = Res.get(); > > > switch ((llvm::bitc::FixedAbbrevIDs)Code) > > > ``` > > > I haven't seen any failures for SerializedDiagnosticReader. So either we > > > lack test coverage for that function, or the sanitizer only introduce the > > > check when using the static_cast (and declaring Code as an enum) as done > > > here. > > > > > That sounds like a pre-existing bug. We should check that the value is in > > range before casting. Can you send patches to fix both code locations, and > > add test coverage? This code is indeed poorly tested. > > > > Why do the sanitizers catch `static_cast` but not C-style casts? > To be clear: relying on the `default` case is still UB because there's a cast > to the enum type before it occurs. I made a patch here (assuming the goal would be to keep the cast to the enum, and to let the switch cover all enum values): https://reviews.llvm.org/D64262 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
