Page MenuHomePhabricator

Fixing DILexicalBlockFile issue in r263424

Authored by aaboud on Mar 22 2016, 6:03 AM.

Diff Detail


Event Timeline

aaboud updated this revision to Diff 51278.Mar 22 2016, 6:03 AM
aaboud retitled this revision from to Fixing DILexicalBlockFile issue in r263424.
aaboud updated this object.
aaboud added a subscriber: llvm-commits.
aprantl edited edge metadata.Mar 22 2016, 8:42 AM

Can you explain the problem that you are running into — why do you need to skip over the lexical block files (my guess is because they are not "real" lexical blocks)? The changes seem straightforward and fine to me, but a little bit of context would be helpful.

129 ↗(On Diff #51278)

Please add a comment here explaining why this is necessary.

Please see the answer below.

129 ↗(On Diff #51278)

I am not sure that I need to add a comment inside the code.
It is just that everywhere else this skip is being done...without any comment.

/// getOrCreateRegularScope - Find or create a regular lexical scope.
LexicalScope *
LexicalScopes::getOrCreateRegularScope(const DILocalScope *Scope) {
  if (auto *File = dyn_cast<DILexicalBlockFile>(Scope))
    Scope = File->getScope();

The below list maps all the local decls to there scope (function or lexical scope), later it will be used when we go over the lexical scopes that where captured by LexicalScopes component (see above), and it does not capture DILexicalBlockFile, but skip it to get the DILexicalBlock.
So, if we do not do the same here we will end up mapping the local decls DIEs to the wrong scope, and they will not be handled later.

If you still think we better comment on this code, please help me capture what I just said above in few words/lines.

aprantl accepted this revision.Mar 22 2016, 2:31 PM
aprantl edited edge metadata.

Putting this comment into the commit message works for me. Thanks!

This revision is now accepted and ready to land.Mar 22 2016, 2:31 PM
aprantl added inline comments.Mar 22 2016, 2:33 PM
4 ↗(On Diff #51278)

Probably don't need to explicitly say this :-)

65 ↗(On Diff #51278)

Please remove all unnecessary attributes.

This revision was automatically updated to reflect the committed changes.
aaboud marked 3 inline comments as done.
rnk added a subscriber: rnk.Mar 24 2016, 1:32 PM
rnk added inline comments.

This assert fails during a build of Chrome for iOS:

Assertion failed: (!isa<DILexicalBlockFile>(Scope) && "Don't expect Lexical Block File!"), function addLocalScopeDieToLexicalScope, file /b/build/slave/ClangToTiOS/build/src/third_party/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp, line 888.
0  clang-3.9                0x000000010b7bed1b llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 43
1  clang-3.9                0x000000010b7bdf36 llvm::sys::RunSignalHandlers() + 70
2  clang-3.9                0x000000010b7bf53f abort + 767
3  libsystem_platform.dylib 0x00007fff9417ff1a _sigtramp + 26
4  clang-3.9                0x000000010daca298 clang::Stmt::StatisticsEnabled + 220476
5  clang-3.9                0x000000010b7bf256 abort + 22
6  clang-3.9                0x000000010b7bf231 __assert_rtn + 81
7  clang-3.9                0x000000010bd277ab llvm::DwarfCompileUnit::addLocalScopeDieToLexicalScope(llvm::LexicalScope*, llvm::DIE*) + 379
8  clang-3.9                0x000000010bd26c66 llvm::DwarfCompileUnit::constructScopeDIE(llvm::LexicalScope*, llvm::SmallVectorImpl<llvm::DIE*>&) + 470
9  clang-3.9                0x000000010bd274fe llvm::DwarfCompileUnit::createScopeChildrenDIE(llvm::LexicalScope*, llvm::SmallVectorImpl<llvm::DIE*>&, bool*) + 686
10 clang-3.9                0x000000010bd26b3b llvm::DwarfCompileUnit::constructScopeDIE(llvm::LexicalScope*, llvm::SmallVectorImpl<llvm::DIE*>&) + 171
11 clang-3.9                0x000000010bd274fe llvm::DwarfCompileUnit::createScopeChildrenDIE(llvm::LexicalScope*, llvm::SmallVectorImpl<llvm::DIE*>&, bool*) + 686
12 clang-3.9                0x000000010bd2884f llvm::DwarfCompileUnit::constructSubprogramScopeDIE(llvm::LexicalScope*) + 287
13 clang-3.9                0x000000010bd344c6 llvm::DwarfDebug::endFunction(llvm::MachineFunction const*) + 1174
14 clang-3.9                0x000000010bd090a4 llvm::AsmPrinter::EmitFunctionBody() + 5764
15 clang-3.9                0x000000010a76ac5c std::__1::__tree<std::__1::__value_type<llvm::MachineInstr*, unsigned int>, std::__1::__map_value_compare<llvm::MachineInstr*, std::__1::__value_type<llvm::MachineInstr*, unsigned int>, std::__1::less<llvm::MachineInstr*>, true>, std::__1::allocator<std::__1::__value_type<llvm::MachineInstr*, unsigned int> > >::destroy(std::__1::__tree_node<std::__1::__value_type<llvm::MachineInstr*, unsigned int>, void*>*) + 2524
16 clang-3.9                0x000000010b19f72c llvm::MachineFunctionPass::runOnFunction(llvm::Function&) + 140
17 clang-3.9                0x000000010b398978 llvm::FPPassManager::runOnFunction(llvm::Function&) + 328
18 clang-3.9                0x000000010b398bcb llvm::FPPassManager::runOnModule(llvm::Module&) + 43
19 clang-3.9                0x000000010b399116 llvm::legacy::PassManagerImpl::run(llvm::Module&) + 998
20 clang-3.9                0x000000010b935f51 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout const&, llvm::Module*, clang::BackendAction, llvm::raw_pwrite_stream*) + 9601
21 clang-3.9                0x000000010bad16ab clang::EmitObjAction::EmitObjAction(llvm::LLVMContext*) + 1547
22 clang-3.9                0x000000010c20bec5 clang::ParseAST(clang::Sema&, bool, bool) + 581
23 clang-3.9                0x000000010bcac80b clang::FrontendAction::Execute() + 75
24 clang-3.9                0x000000010bc71bc1 clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) + 1073
25 clang-3.9                0x000000010bcf0149 clang::ExecuteCompilerInvocation(clang::CompilerInstance*) + 4713
26 clang-3.9                0x000000010a5197d6 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) + 1366
echristo edited edge metadata.Mar 24 2016, 1:34 PM
echristo added a subscriber: echristo.

Since we have a public (if inconvenient) reproduction, can one of you
please revert for now?