jfb marked an inline comment as done. jfb 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: > 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. 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
