[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[Erik Pilkington via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b529e311a90: [ObjC] Fix AST serialization for pseudo-strong 
parameters (authored by dgoldman, committed by erik.pilkington).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417

Files:
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/externally-retained.m


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t 
-debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1100,8 +1100,6 @@
 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.
@@ -2121,7 +2119,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // SClass
   Abv->Add(BitCodeAbbrevOp(0));   // TSCSpec
   Abv->Add(BitCodeAbbrevOp(0));   // InitStyle
-  Abv->Add(BitCodeAbbrevOp(0));   // ARCPseudoStrong
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));   // Linkage
   Abv->Add(BitCodeAbbrevOp(0));   // HasInit
   Abv->Add(BitCodeAbbrevOp(0));   // 
HasMemberSpecializationInfo


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t -debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1100,8 +1100,6 @@
 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.
@@ -2121,7 +2119,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // SClass
   Abv->Add(BitCodeAbbrevOp(0));   // TSCSpec
   Abv->Add(BitCodeAbbrevOp(0));   // InitStyle
-  Abv->Add(BitCodeAbbrevOp(0));   // ARCPseudoStrong
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));   // Linkage
   Abv->Add(BitCodeAbbrevOp(0));   // HasInit
   Abv->Add(BitCodeAbbrevOp(0));   

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington accepted this revision.
erik.pilkington added a comment.
This revision is now accepted and ready to land.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 244728.
dgoldman added a comment.

- Fix parameter abbreviation for ParamVarDecl


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417

Files:
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/externally-retained.m


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t 
-debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.
@@ -2110,7 +2108,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // SClass
   Abv->Add(BitCodeAbbrevOp(0));   // TSCSpec
   Abv->Add(BitCodeAbbrevOp(0));   // InitStyle
-  Abv->Add(BitCodeAbbrevOp(0));   // ARCPseudoStrong
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));   // Linkage
   Abv->Add(BitCodeAbbrevOp(0));   // HasInit
   Abv->Add(BitCodeAbbrevOp(0));   // 
HasMemberSpecializationInfo


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t -debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.
@@ -2110,7 +2108,7 @@
   Abv->Add(BitCodeAbbrevOp(0));   // SClass
   Abv->Add(BitCodeAbbrevOp(0));   // TSCSpec
   Abv->Add(BitCodeAbbrevOp(0));   // InitStyle
-  Abv->Add(BitCodeAbbrevOp(0));   // ARCPseudoStrong
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong
   Abv->Add(BitCodeAbbrevOp(0));   // Linkage
   Abv->Add(BitCodeAbbrevOp(0));   // HasInit
   Abv->Add(BitCodeAbbrevOp(0));   // HasMemberSpecializationInfo
___
cfe-commits mailing 

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 244467.
dgoldman added a comment.

- Add test (fails due to different assertion)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417

Files:
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/PCH/externally-retained.m


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t 
-debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.


Index: clang/test/PCH/externally-retained.m
===
--- /dev/null
+++ clang/test/PCH/externally-retained.m
@@ -0,0 +1,30 @@
+// Test for assertion failure due to objc_externally_retained on a function.
+
+// Without PCH
+// RUN: %clang_cc1 -fsyntax-only -verify -fobjc-arc -include %s %s
+
+// With PCH
+// RUN: %clang_cc1 %s -emit-pch -fobjc-arc -o %t
+// RUN: %clang_cc1 -emit-llvm-only -verify %s -fobjc-arc -include-pch %t -debug-info-kind=limited
+
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+//===--===//
+// Header
+
+__attribute__((objc_externally_retained)) void doSomething(id someObject);
+
+id sharedObject = 0;
+
+//===--===//
+#else
+//===--===//
+
+void callDoSomething() {
+  doSomething(sharedObject);
+}
+
+//===--===//
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

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. 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  clang0x00010aa9515c 
llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 60
  1  clang0x00010aa95719 
PrintStackTraceSignalHandler(void*) + 25
  2  clang0x00010aa93146 llvm::sys::RunSignalHandlers() 
+ 118
  3  clang0x00010aa992fc SignalHandler(int) + 252
  4  libsystem_platform.dylib 0x7fff67583b5d _sigtramp + 29
  5  clang0x0001122350d8 
llvm::DenseMapInfo::Tombstone + 3253576
  6  libsystem_c.dylib0x7fff6743d6a6 abort + 127
  7  libsystem_c.dylib0x7fff6740620d basename_r + 0
  8  clang0x00010bc4e7f3 void 
llvm::BitstreamWriter::EmitAbbreviatedLiteral(llvm::BitCodeAbbrevOp const&, unsigned long long) + 211
  9  clang0x00010bc4de0c void 
llvm::BitstreamWriter::EmitRecordWithAbbrevImpl(unsigned 
int, llvm::ArrayRef, llvm::StringRef, 
llvm::Optional) + 892
  10 clang0x00010bc4d863 void 
llvm::BitstreamWriter::EmitRecord 
>(unsigned int, llvm::SmallVectorImpl const&, unsigned int) 
+ 307
  11 clang0x00010bebb274 
clang::ASTRecordWriter::Emit(unsigned int, unsigned int) + 84
  12 clang0x00010bf3e19b 
clang::ASTDeclWriter::Emit(clang::Decl*) + 203
  13 clang0x00010bf3de25 
clang::ASTWriter::WriteDecl(clang::ASTContext&, clang::Decl*) + 517
  ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added a comment.

This looks good, but please add a testcase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74417/new/

https://reviews.llvm.org/D74417



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74417: [clang][ARC] Remove invalid assertion that can crash clangd

[David Goldman via Phabricator via cfe-commits]

dgoldman created this revision.
dgoldman added reviewers: erik.pilkington, jkorous.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, 
dexonsmith, MaskRay, ilya-biryukov.
Herald added a project: clang.

- 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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74417

Files:
  clang/lib/Serialization/ASTWriterDecl.cpp


Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.


Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1089,8 +1089,6 @@
 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.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits