Fix MetadataLoader treatment of NumExtraInhabitants. #7684
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After 89d458b and swiftlang#7662, the code of MetadataLoader.cpp allows the new NumExtraInhabitants field, but it always supposes it is available. Some tests do not seem to like that. When compiling with asserts, binaries like `opt` will crash with an assertion accessing `SmallVector` indices out of bounts, while in no-asserts it just keeps going, probably using garbage values. Change two places where the `Record` was accessed with `operator[]` without first checking for the `Record` size to check for the size. Use a default of `0` in case the record is not found. The test that failed before these changes are: ``` LLVM :: Bitcode/DIExpression-4.0.ll LLVM :: Bitcode/DIExpression-aggresult.ll LLVM :: Bitcode/DIExpression-deref.ll LLVM :: Bitcode/DIExpression-minus-upgrade.ll LLVM :: Bitcode/DIGlobalVariableExpression.ll LLVM :: Bitcode/DIGlobalVariableExpression2.ll LLVM :: Bitcode/DIModule-fortran-module.ll LLVM :: Bitcode/DINamespace.ll LLVM :: Bitcode/DISubprogram-v4.ll LLVM :: Bitcode/DISubprogram-v5.ll LLVM :: Bitcode/DITemplateParameter-5.0.ll LLVM :: Bitcode/diglobalvariable-3.8.ll LLVM :: Bitcode/dilocalvariable-3.9.ll LLVM :: Bitcode/disubrange-v0.ll LLVM :: Bitcode/dityperefs-3.8.ll LLVM :: Bitcode/invalid.test LLVM :: Bitcode/upgrade-cu-locals.ll LLVM :: Bitcode/upgrade-dbg-addr.ll LLVM :: Bitcode/upgrade-dbg-value.ll LLVM :: Bitcode/upgrade-pointer-address-space.ll LLVM :: ThinLTO/X86/drop-debug-info.ll ``` The failure looked like the following: ``` Assertion failed: (idx < size()), function operator[], file SmallVector.h, line 294. PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: /Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis -o - /Users/danielrodriguez/code/swift-source/llvm-project/llvm/test/Bitcode/DIExpression-minus-upgrade.ll.bc #0 0x00000001003507b7 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x1001ca7b7) swiftlang#1 0x000000010034ed55 llvm::sys::RunSignalHandlers() (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x1001c8d55) swiftlang#2 0x0000000100351040 SignalHandler(int) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x1001cb040) swiftlang#3 0x00007ff805ec05ed (/usr/lib/system/libsystem_platform.dylib+0x7ff8004245ed) swiftlang#4 0x0000000000000000 swiftlang#5 0x00007ff805db9b45 (/usr/lib/system/libsystem_c.dylib+0x7ff80031db45) swiftlang#6 0x00007ff805db8e5e (/usr/lib/system/libsystem_c.dylib+0x7ff80031ce5e) swiftlang#7 0x0000000100385573 llvm::MetadataLoader::MetadataLoaderImpl::parseOneMetadata(llvm::SmallVectorImpl<unsigned long long>&, unsigned int, (anonymous namespace)::(anonymous namespace)::PlaceholderQueue&, llvm::StringRef, unsigned int&) (.cold.50) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x1001ff573) swiftlang#8 0x00000001001d13a3 llvm::MetadataLoader::MetadataLoaderImpl::parseOneMetadata(llvm::SmallVectorImpl<unsigned long long>&, unsigned int, (anonymous namespace)::(anonymous namespace)::PlaceholderQueue&, llvm::StringRef, unsigned int&) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x10004b3a3) swiftlang#9 0x00000001001c9496 llvm::MetadataLoader::MetadataLoaderImpl::parseMetadata(bool) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x100043496) swiftlang#10 0x00000001001d2f71 llvm::MetadataLoader::parseMetadata(bool) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x10004cf71) swiftlang#11 0x00000001001b33b7 (anonymous namespace)::BitcodeReader::parseModule(unsigned long long, bool, llvm::ParserCallbacks) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x10002d3b7) swiftlang#12 0x000000010018f2f5 llvm::BitcodeModule::getModuleImpl(llvm::LLVMContext&, bool, bool, bool, llvm::ParserCallbacks) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x1000092f5) swiftlang#13 0x000000010019008a llvm::BitcodeModule::getLazyModule(llvm::LLVMContext&, bool, bool, llvm::ParserCallbacks) (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x10000a08a) swiftlang#14 0x0000000100189593 main (/Users/danielrodriguez/code/swift-source/build/my_macos/llvm-macosx-x86_64/bin/llvm-dis+0x100003593) swiftlang#15 0x0000000200a0941f ``` With these changes, those tests all pass and the crash does not reproduce.
|
@swift-ci please test |
augusto2112
approved these changes
Oct 25, 2023
|
@drodriguez thanks!! |
drodriguez
changed the title
|
@swift-ci please test Linux platform |
|
I do not have permission to land in the llvm-project. Feel free to land for me. All builds seem green. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After 89d458b and #7662, the code of MetadataLoader.cpp allows the new NumExtraInhabitants field, but it always supposes it is available. Some tests do not seem to like that. When compiling with asserts, binaries like
optwill crash with an assertion accessingSmallVectorindices out of bounds, while in no-asserts it just keeps going, probably using garbage values.Change two places where the
Recordwas accessed withoperator[]without first checking for theRecordsize to check for the size. Use a default of0in case the record is not found.The test that failed before these changes are:
The failure looked like the following:
With these changes, those tests all pass and the crash does not reproduce.
/cc @augusto2112