[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D83997#2169745 , @ahatanak wrote:

> The use case for this is a macro in which the call to 
> `__builtin_os_log_format` that writes to the buffer and the call that uses 
> the buffer appear in two different statements. For example:
>
>   __builtin_os_log_format(buf, "%@", getObj());
>   ...
>   use_buffer(buf);
>
> The object returned by the call to `getObj` has to be kept alive until 
> `use_buffer` is called, but currently it gets destructed at the end of the 
> full expression. I think an alternate solution would be to provide users a 
> means to tell ARC optimizer not to move the release call for a local variable 
> past any calls, i.e., something that is stricter than 
> `NS_VALID_UNTIL_END_OF_SCOPE`, but that places more burden on the users.
>
> In the `os_log` macro, the result of the call to `__builtin_os_log_format` is 
> passed directly to the call that uses the buffer, so it doesn't require any 
> lifetime extension as you pointed out.

So are there actually any uses that take advantage of these semantics?  Because 
as I understand it, this builtin exists entirely to support the `os_log` macro. 
 If that macro doesn't need the extension semantics, let's just not do them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83997

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

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:8000
   return ExprWithCleanups::CleanupObject(cast(*R));
+  } else if (auto *ICE = From.dyn_cast()) {
+if (Expected R = Import(ICE))

I am not sure how this hunk is related to `os_log`. Maybe you mistakenly mixed 
another unrelated change into this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83997



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

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

The use case for this is a macro in which the call to `__builtin_os_log_format` 
that writes to the buffer and the call that uses the buffer appear in two 
different statements. For example:

  __builtin_os_log_format(buf, "%@", getObj());
  ...
  use_buffer(buf);

The object returned by the call to `getObj` has to be kept alive until 
`use_buffer` is called, but currently it gets destructed at the end of the full 
expression. I think an alternate solution would be to provide users a means to 
tell ARC optimizer not to move the release call for a local variable past any 
calls, i.e., something that is stricter than `NS_VALID_UNTIL_END_OF_SCOPE`, but 
that places more burden on the users.

In the `os_log` macro, the result of the call to `__builtin_os_log_format` is 
passed directly to the call that uses the buffer, so it doesn't require any 
lifetime extension as you pointed out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83997



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

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Why is the lifetime extended to the enclosing block scope anyway?  I understand 
why we need a clang.arc.use — the optimizer can't reasonably understand that 
the object has to live within the buffer — but isn't the buffer only used for 
the duration of the call?  Why is extension necessary?




Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1987
+  COK_CompoundLiteral,
+  COK_ImplicitCastExpr
+};

This seems like an excessively general name for what's happening here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83997



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

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added reviewers: rjmccall, erik.pilkington.
ahatanak added a project: clang.
Herald added subscribers: ributzka, dexonsmith, jkorous, martong.
Herald added a reviewer: shafik.

This patch fixes several shortcomings of the way we currently extend the 
lifetime of objects passed to calls to __builtin_os_log_format:

- Sema doesn't diagnose code that jumps into or out of the scope of a 
lifetime-extended object.
- Lifetime of objects in comma operators or expressions for ObjC property 
access isn't extended.
- We extend the lifetime of an object by emitting an extra pair of retain and 
release calls instead of delaying the release call.
- Calls to clang.arc.use are emitted on exception paths even though they are 
needed only on normal paths.

rdar://problem/62610583


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83997

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/JSONNodeDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGExprAgg.cpp
  clang/lib/CodeGen/CGExprComplex.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/JumpDiagnostics.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/test/AST/ast-dump-objc-arc-json.m
  clang/test/AST/ast-dump-stmt.m
  clang/test/CodeGenObjC/os_log.m
  clang/test/CodeGenObjCXX/os_log.mm
  clang/test/PCH/Inputs/arc.h
  clang/test/PCH/arc.m
  clang/test/SemaObjC/format-strings-oslog.m

Index: clang/test/SemaObjC/format-strings-oslog.m
===
--- clang/test/SemaObjC/format-strings-oslog.m
+++ clang/test/SemaObjC/format-strings-oslog.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -verify %s
 
 #include 
 #include 
@@ -6,6 +6,7 @@
 #include  // For wint_t and wchar_t
 
 int printf(const char *restrict, ...);
+id getObj(void);
 
 @interface NSString
 @end
@@ -67,3 +68,38 @@
   MyOSLog("%d");// expected-warning {{more '%' conversions than data arguments}}
   MyOSLog("%P", p); // expected-warning {{using '%P' format specifier without precision}}
 }
+
+void test_arc_extended_lifetime0(int cond, void *buf) {
+  switch (cond) {
+  case 0:
+__builtin_os_log_format(buf, @"%@", getObj()); // expected-note {{jump enters lifetime of an object}}
+break;
+  default: // expected-error {{cannot jump from switch statement to this case label}}
+break;
+  }
+}
+
+void test_arc_extended_lifetime1(int cond, void *buf, id a) {
+  switch (cond) {
+  case 0:
+__builtin_os_log_format(buf, @"%@", a);
+break;
+  default:
+break;
+  }
+}
+
+void test_arc_extended_lifetime2(int cond, void *buf) {
+  static void *ips[] = {&};
+L0: // expected-note {{possible target of indirect goto}}
+;
+  __builtin_os_log_format(buf, @"%@", getObj()); // expected-note {{jump exits lifetime of an object}}
+  goto *ips; // expected-error {{cannot jump}}
+}
+
+void test_arc_extended_lifetime3(int cond, void *buf, id a) {
+  static void *ips[] = {&};
+L0:;
+  __builtin_os_log_format(buf, @"%@", a);
+  goto *ips;
+}
Index: clang/test/PCH/arc.m
===
--- clang/test/PCH/arc.m
+++ clang/test/PCH/arc.m
@@ -4,7 +4,7 @@
 
 // Test with pch.
 // RUN: %clang_cc1 -emit-pch -fblocks -triple x86_64-apple-darwin11 -fobjc-arc -x objective-c-header -o %t %S/Inputs/arc.h
-// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -fobjc-arc -include-pch %t -fsyntax-only -emit-llvm-only %s 
+// RUN: %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -fobjc-arc -include-pch %t -emit-llvm -o - %s | FileCheck %s
 
 // Test error when pch's -fobjc-arc state is different.
 // RUN: not %clang_cc1 -fblocks -triple x86_64-apple-darwin11 -include-pch %t -fsyntax-only -emit-llvm-only %s 2>&1 | FileCheck -check-prefix=CHECK-ERR1 %s 
@@ -16,3 +16,15 @@
 
 // CHECK-ERR1: Objective-C automated reference counting was enabled in PCH file but is currently disabled
 // CHECK-ERR2: Objective-C automated reference counting was disabled in PCH file but is currently enabled
+
+// CHECK: define internal void @callBuiltinOSLogFormat(
+// CHECK: %[[CALL:.*]] = call i8* @getObj()
+// CHECK: %[[V1:.*]] = notail call i8* @llvm.objc.retainAutoreleasedReturnValue(i8* %[[CALL]])
+// CHECK: %[[V2:.*]] = ptrtoint i8* %[[V1]] to i64
+// CHECK: call void @__os_log_helper_1_2_1_8_64(i8* %{{.*}}, i64 %[[V2]])
+// CHECK: call void @os_log_pack_send(
+// CHECK: call void @llvm.objc.release(i8* %[[V1]])
+
+void testBuiltinOSLogFormat(void) {
+  callBuiltinOSLogFormat(0);
+}
Index: