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

[dgoldman]

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

[erik.pilkington]

This looks good, but please add a testcase.

[dgoldman]

• Add test (fails due to different assertion)

[dgoldman]

In D74417#1873287, @erik.pilkington wrote:

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
...

[erik.pilkington]

In D74417#1874747, @dgoldman wrote:

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]

• Fix parameter abbreviation for ParamVarDecl

[dgoldman]

In D74417#1874826, @erik.pilkington wrote:

In D74417#1874747, @dgoldman wrote:

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]

In D74417#1876927, @dgoldman wrote:

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!

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTDeclWriter::VisitParmVarDecl(ParmVarDecl *D) {
   Record.push_back(D->getObjCDeclQualifier()); // FIXME: stable encoding
   Record.push_back(D->isKNRPromoted());
   Record.push_back(D->hasInheritedDefaultArg());
   Record.push_back(D->hasUninstantiatedDefaultArg());
   if (D->hasUninstantiatedDefaultArg())
     Record.AddStmt(D->getUninstantiatedDefaultArg());
   Code = serialization::DECL_PARM_VAR;
 
-  assert(!D->isARCPseudoStrong()); // can be true of ImplicitParamDecl
-
   // If the assumptions about the DECL_PARM_VAR abbrev are true, use it.  Here
   // we dynamically check for the properties that we optimize for, but don't
   // know are true of all PARM_VAR_DECLs.
   if (D->getDeclContext() == D->getLexicalDeclContext() &&
       !D->hasAttrs() &&
       !D->hasExtInfo() &&
       !D->isImplicit() &&
       !D->isUsed(false) &&