Page MenuHomePhabricator

[clang][ARC] Remove invalid assertion that can crash clangd
AcceptedPublic

Authored by dgoldman on Feb 11 2020, 8:36 AM.

Details

Summary
  • This assertion will fire when functions are marked with objc_externally_retained, such as in the following function: __attribute__((objc_externally_retained)) void AssertOnQueue(dispatch_queue_t queue)
  • Thus the assertion isn't valid as these parameters are indeed psuedo strong

Event Timeline

dgoldman created this revision.Feb 11 2020, 8:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2020, 8:36 AM

This looks good, but please add a testcase.

dgoldman updated this revision to Diff 244467.Feb 13 2020, 10:02 AM
  • Add test (fails due to different assertion)

This looks good, but please add a testcase.

Added but it's still failing due to a different assertion failure, do you think this could be because the abbreviation is different for the ParamVars? I'm not sure how to handle this...

Assertion failed: (V == Op.getLiteralValue() && "Invalid abbrev for record!"), function EmitAbbreviatedLiteral, file /Users/davg/dev/git_llvm/source/llvm-project/llvm/include/llvm/Bitstream/BitstreamWriter.h, line 263.
Stack dump:
0.	Program arguments: /Users/davg/dev/git_llvm/build/bin/clang -cc1 -internal-isystem /Users/davg/dev/git_llvm/build/lib/clang/11.0.0/include -nostdsysteminc /Users/davg/dev/git_llvm/source/llvm-project/clang/test/PCH/externally-retained.m -emit-pch -fobjc-arc -o /Users/davg/dev/git_llvm/build/tools/clang/test/PCH/Output/externally-retained.m.tmp
1.	<eof> parser at end of file
2.	/Users/davg/dev/git_llvm/source/llvm-project/clang/test/PCH/externally-retained.m:17:63: serializing 'someObject'
0  clang                    0x000000010aa9515c llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
1  clang                    0x000000010aa95719 PrintStackTraceSignalHandler(void*) + 25
2  clang                    0x000000010aa93146 llvm::sys::RunSignalHandlers() + 118
3  clang                    0x000000010aa992fc SignalHandler(int) + 252
4  libsystem_platform.dylib 0x00007fff67583b5d _sigtramp + 29
5  clang                    0x00000001122350d8 llvm::DenseMapInfo<llvm::codeview::GloballyHashedType>::Tombstone + 3253576
6  libsystem_c.dylib        0x00007fff6743d6a6 abort + 127
7  libsystem_c.dylib        0x00007fff6740620d basename_r + 0
8  clang                    0x000000010bc4e7f3 void llvm::BitstreamWriter::EmitAbbreviatedLiteral<unsigned long long>(llvm::BitCodeAbbrevOp const&, unsigned long long) + 211
9  clang                    0x000000010bc4de0c void llvm::BitstreamWriter::EmitRecordWithAbbrevImpl<unsigned long long>(unsigned int, llvm::ArrayRef<unsigned long long>, llvm::StringRef, llvm::Optional<unsigned int>) + 892
10 clang                    0x000000010bc4d863 void llvm::BitstreamWriter::EmitRecord<llvm::SmallVectorImpl<unsigned long long> >(unsigned int, llvm::SmallVectorImpl<unsigned long long> const&, unsigned int) + 307
11 clang                    0x000000010bebb274 clang::ASTRecordWriter::Emit(unsigned int, unsigned int) + 84
12 clang                    0x000000010bf3e19b clang::ASTDeclWriter::Emit(clang::Decl*) + 203
13 clang                    0x000000010bf3de25 clang::ASTWriter::WriteDecl(clang::ASTContext&, clang::Decl*) + 517
...

Added but it's still failing due to a different assertion failure, do you think this could be because the abbreviation is different for the ParamVars? I'm not sure how to handle this...

Yeah, that looks to be the problem, the parameter abbreviation is assumed to be a literal zero (since it was previously impossible) for the ParmVarDecl case, i.e:

Abv->Add(BitCodeAbbrevOp(0));                       // ARCPseudoStrong

But we really want it to look like the VarDecl case, where we actually get a bit for it:

Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong

I think you can fix the crash by changing the BitCodeAbbrevOp(0) to BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1).

dgoldman updated this revision to Diff 244728.Feb 14 2020, 11:45 AM
  • Fix parameter abbreviation for ParamVarDecl

Added but it's still failing due to a different assertion failure, do you think this could be because the abbreviation is different for the ParamVars? I'm not sure how to handle this...

Yeah, that looks to be the problem, the parameter abbreviation is assumed to be a literal zero (since it was previously impossible) for the ParmVarDecl case, i.e:

Abv->Add(BitCodeAbbrevOp(0));                       // ARCPseudoStrong

But we really want it to look like the VarDecl case, where we actually get a bit for it:

Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong

I think you can fix the crash by changing the BitCodeAbbrevOp(0) to BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1).

Okay, I've added this, it seems to fix the test but I'm not sure if it's fully correct since the place where it was modified is under the VarDecl comment instead of ParmVarDecl.

erik.pilkington accepted this revision.Feb 28 2020, 4:12 PM

Okay, I've added this, it seems to fix the test but I'm not sure if it's fully correct since the place where it was modified is under the VarDecl comment instead of ParmVarDecl.

It looks like it's in ParmVarDecl to me, its just in the VarDecl region of the complete record. ParmVarDecl inherits from VarDecl, so when parameters are pseudo-strong, that information is stored in the bit in the VarDecl. Thanks for fixing this!

This revision is now accepted and ready to land.Feb 28 2020, 4:12 PM