Skip to content

[Clang][Interp] __builtin_os_log_format_buffer_size should be an unevaluated builtin #99895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 23, 2024
Merged

[Clang][Interp] __builtin_os_log_format_buffer_size should be an unevaluated builtin #99895

merged 1 commit into from
Jul 23, 2024

Conversation

yronglin
Copy link
Contributor

Follow the current behavior of constant evaluator, __builtin_os_log_format_buffer_size should be an unevaluated builtin.

The following code is well-formed:

void test_builtin_os_log(void *buf, int i, const char *data) {
  constexpr int len = __builtin_os_log_format_buffer_size("%d %{public}s %{private}.16P", i, data, data);
}

@yronglin yronglin requested a review from tbaederr as a code owner July 22, 2024 16:48
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 22, 2024

@llvm/pr-subscribers-clang

Author: None (yronglin)

Changes

Follow the current behavior of constant evaluator, __builtin_os_log_format_buffer_size should be an unevaluated builtin.

The following code is well-formed:

void test_builtin_os_log(void *buf, int i, const char *data) {
  constexpr int len = __builtin_os_log_format_buffer_size("%d %{public}s %{private}.16P", i, data, data);
}

Full diff: https://github.com/llvm/llvm-project/pull/99895.diff

2 Files Affected:

  • (modified) clang/lib/AST/Interp/ByteCodeEmitter.cpp (+2-1)
  • (modified) clang/test/CodeGen/builtins.c (+1)
diff --git a/clang/lib/AST/Interp/ByteCodeEmitter.cpp b/clang/lib/AST/Interp/ByteCodeEmitter.cpp
index a3d4c7d7392da..fee4432a8f661 100644
--- a/clang/lib/AST/Interp/ByteCodeEmitter.cpp
+++ b/clang/lib/AST/Interp/ByteCodeEmitter.cpp
@@ -27,7 +27,8 @@ using namespace clang::interp;
 /// Similar information is available via ASTContext::BuiltinInfo,
 /// but that is not correct for our use cases.
 static bool isUnevaluatedBuiltin(unsigned BuiltinID) {
-  return BuiltinID == Builtin::BI__builtin_classify_type;
+  return BuiltinID == Builtin::BI__builtin_classify_type ||
+         BuiltinID == Builtin::BI__builtin_os_log_format_buffer_size;
 }
 
 Function *ByteCodeEmitter::compileFunc(const FunctionDecl *FuncDecl) {
diff --git a/clang/test/CodeGen/builtins.c b/clang/test/CodeGen/builtins.c
index b41efb59e61db..1f998236501d2 100644
--- a/clang/test/CodeGen/builtins.c
+++ b/clang/test/CodeGen/builtins.c
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -emit-llvm -o %t %s
 // RUN: not grep __builtin %t
 // RUN: %clang_cc1 -emit-llvm -triple x86_64-darwin-apple -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-darwin-apple -fexperimental-new-constant-interpreter -o - %s | FileCheck %s
 
 int printf(const char *, ...);

tbaederr
tbaederr approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@tbaederr tbaederr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@yronglin yronglin merged commit 4572efe into llvm:main Jul 23, 2024
10 checks passed
@hctim
Copy link
Collaborator

hctim commented Jul 23, 2024

Hi,

Unfortunately this patch appears to have introduced a memory leak that was detected by the sanitizer buildbots (https://lab.llvm.org/buildbot/#/builders/52/builds/1079).

The issue can be reproduced with a minimal ASan build configuration, like:

$ cmake \
-DCMAKE_C_COMPILER=clang \
-DCMAKE_CXX_COMPILER=clang++ \
-DLLVM_USE_LINKER=lld \
-GNinja \
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_C_FLAGS="-fsanitize=address" \
-DCMAKE_CXX_FLAGS="-fsanitize=address" \
-DLLVM_USE_SANITIZER=Address \
/path/to/llvm/llvm

$ LIT_OPTS='--filter builtins' ninja check-clang
FAIL: Clang :: CodeGen/builtins.c (501 of 506)                                                               11:15:42 [210/894]
******************** TEST 'Clang :: CodeGen/builtins.c' FAILED ********************                                            
Exit Code: 1                                                                                                                   
                                                                                                                               
Command Output (stderr):                                                                                                       
--                                                                                                                             
RUN: at line 1: /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitc
hp/llvm-build/asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -o /usr/local/google/home/mitchp/llvm-build/asan-test/t
ools/clang/test/CodeGen/Output/builtins.c.tmp /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c                 
+ /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitchp/llvm-build/
asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -o /usr/local/google/home/mitchp/llvm-build/asan-test/tools/clang/tes
t/CodeGen/Output/builtins.c.tmp /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c                               
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:52: warning: invalid conversion specifier ':' [-Wformat-in
valid-specifier]                                                                                                               
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);                                  
      |                                                   ~^                                                                   
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:76: warning: incomplete format specifier [-Wformat]       
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);                                  
      |                                                                            ^                                           
2 warnings generated.                                                                                                          
RUN: at line 2: not grep __builtin /usr/local/google/home/mitchp/llvm-build/asan-test/tools/clang/test/CodeGen/Output/builtins.
c.tmp                                                                                                                          
+ not grep __builtin /usr/local/google/home/mitchp/llvm-build/asan-test/tools/clang/test/CodeGen/Output/builtins.c.tmp         
RUN: at line 3: /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitc
hp/llvm-build/asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -triple x86_64-darwin-apple -o - /usr/local/google/home
/mitchp/llvm/clang/test/CodeGen/builtins.c | /usr/local/google/home/mitchp/llvm-build/asan-test/bin/FileCheck /usr/local/google
/home/mitchp/llvm/clang/test/CodeGen/builtins.c                                                                                
+ /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitchp/llvm-build/
asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -triple x86_64-darwin-apple -o - /usr/local/google/home/mitchp/llvm/c
lang/test/CodeGen/builtins.c
+ /usr/local/google/home/mitchp/llvm-build/asan-test/bin/FileCheck /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builti
ns.c
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:52: warning: invalid conversion specifier ':' [-Wformat-in
valid-specifier]
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);
      |                                                   ~^
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:76: warning: incomplete format specifier [-Wformat]
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);
      |                                                                            ^
2 warnings generated.
RUN: at line 4: /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitc
hp/llvm-build/asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -triple x86_64-darwin-apple -fexperimental-new-constant
-interpreter -o - /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c | /usr/local/google/home/mitchp/llvm-build/a
san-test/bin/FileCheck /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c
+ /usr/local/google/home/mitchp/llvm-build/asan-test/bin/clang -cc1 -internal-isystem /usr/local/google/home/mitchp/llvm-build/
asan-test/lib/clang/19/include -nostdsysteminc -emit-llvm -triple x86_64-darwin-apple -fexperimental-new-constant-interpreter -
o - /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c
+ /usr/local/google/home/mitchp/llvm-build/asan-test/bin/FileCheck /usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builti
ns.c
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:52: warning: invalid conversion specifier ':' [-Wformat-in
valid-specifier]
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);
      |                                                   ~^
/usr/local/google/home/mitchp/llvm/clang/test/CodeGen/builtins.c:772:76: warning: incomplete format specifier [-Wformat]
  772 |   __builtin_os_log_format(buf, "invalid specifier %: %d even a trailing one%", data);
      |                                                                            ^
2 warnings generated.

=================================================================
==2982256==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x5649a842c99d in operator new[](unsigned long) /usr/local/google/home/mitchp/llvm/compiler-rt/lib/asan/asan_new_delete.
cpp:89:3
    #1 0x5649aeb6ca36 in initialize /usr/local/google/home/mitchp/llvm/llvm/lib/Support/APFloat.cpp:882:25
    #2 0x5649aeb6ca36 in llvm::detail::IEEEFloat::IEEEFloat(llvm::detail::IEEEFloat const&) /usr/local/google/home/mitchp/llvm/
llvm/lib/Support/APFloat.cpp:1171:3
    #3 0x5649b7fc7960 in Storage /usr/local/google/home/mitchp/llvm/llvm/include/llvm/ADT/APFloat.h:855:20
    #4 0x5649b7fc7960 in APFloat /usr/local/google/home/mitchp/llvm/llvm/include/llvm/ADT/APFloat.h:978:3
    #5 0x5649b7fc7960 in Floating /usr/local/google/home/mitchp/llvm/clang/lib/AST/Interp/Floating.h:26:7
    #6 0x5649b7fc7960 in push<clang::interp::Floating, clang::interp::Floating &> /usr/local/google/home/mitchp/llvm/clang/lib/
AST/Interp/InterpStack.h:36:35
    #7 0x5649b7fc7960 in clang::interp::CastFP(clang::interp::InterpState&, clang::interp::CodePtr, llvm::fltSemantics const*, 
llvm::RoundingMode) /usr/local/google/home/mitchp/llvm/clang/lib/AST/Interp/Interp.h:2018:9
    #8 0x5649b7e102a8 in clang::interp::Compiler<clang::interp::EvalEmitter>::VisitCastExpr(clang::CastExpr const*) /usr/local/
google/home/mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:287:18
    #9 0x5649b7e12409 in clang::interp::Compiler<clang::interp::EvalEmitter>::visit(clang::Expr const*) /usr/local/google/home/
mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:3248:16
    #10 0x5649b7e1e52b in clang::interp::Compiler<clang::interp::EvalEmitter>::VisitBuiltinCallExpr(clang::CallExpr const*) /us
r/local/google/home/mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:3911:18
    #11 0x5649b7e1c155 in clang::interp::Compiler<clang::interp::EvalEmitter>::VisitCallExpr(clang::CallExpr const*) /usr/local
/google/home/mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:3930:12
    #12 0x5649b7e12409 in clang::interp::Compiler<clang::interp::EvalEmitter>::visit(clang::Expr const*) /usr/local/google/home
/mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:3248:16
    #13 0x5649b7e45c6b in clang::interp::Compiler<clang::interp::EvalEmitter>::visitExpr(clang::Expr const*) /usr/local/google/
home/mitchp/llvm/clang/lib/AST/Interp/Compiler.cpp:3561:10
    #14 0x5649b7f5fd30 in clang::interp::EvalEmitter::interpretExpr(clang::Expr const*, bool) /usr/local/google/home/mitchp/llv
m/clang/lib/AST/Interp/EvalEmitter.cpp:47:14
    #15 0x5649b7dc80e1 in clang::interp::Context::evaluateAsRValue(clang::interp::State&, clang::Expr const*, clang::APValue&) 
/usr/local/google/home/mitchp/llvm/clang/lib/AST/Interp/Context.cpp:49:16
    #16 0x5649b7c0d5c2 in EvaluateAsRValue((anonymous namespace)::EvalInfo&, clang::Expr const*, clang::APValue&) /usr/local/go
ogle/home/mitchp/llvm/clang/lib/AST/ExprConstant.cpp:15843:38
    #17 0x5649b7c019eb in EvaluateAsRValue /usr/local/google/home/mitchp/llvm/clang/lib/AST/ExprConstant.cpp:15920:10
    #18 0x5649b7c019eb in clang::Expr::EvaluateAsRValue(clang::Expr::EvalResult&, clang::ASTContext const&, bool) const /usr/lo
cal/google/home/mitchp/llvm/clang/lib/AST/ExprConstant.cpp:15969:10
    #19 0x5649afc2c67c in clang::CodeGen::CodeGenFunction::EmitBuiltinExpr(clang::GlobalDecl, unsigned int, clang::CallExpr con
st*, clang::CodeGen::ReturnValueSlot) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGBuiltin.cpp:2553:28
    #20 0x5649af6897e7 in clang::CodeGen::CodeGenFunction::EmitCallExpr(clang::CallExpr const*, clang::CodeGen::ReturnValueSlot
) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExpr.cpp:5468:12
    #21 0x5649af6d1c41 in (anonymous namespace)::ScalarExprEmitter::VisitCallExpr(clang::CallExpr const*) /usr/local/google/hom
e/mitchp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:597:20
    #22 0x5649af699aab in (anonymous namespace)::ScalarExprEmitter::Visit(clang::Expr*) /usr/local/google/home/mitchp/llvm/clan
g/lib/CodeGen/CGExprScalar.cpp:422:52
    #23 0x5649af6ea1e1 in (anonymous namespace)::ScalarExprEmitter::VisitCastExpr(clang::CastExpr*) /usr/local/google/home/mitc
hp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:2642:33
    #24 0x5649af699838 in Visit /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:422:52
    #25 0x5649af699838 in clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) /usr/local/google/home/mitc
hp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:5497:8
    #26 0x5649af62cd46 in clang::CodeGen::CodeGenFunction::EmitAnyExpr(clang::Expr const*, clang::CodeGen::AggValueSlot, bool) 
/usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExpr.cpp:230:24
    #27 0x5649af62e4b8 in clang::CodeGen::CodeGenFunction::EmitAnyExprToTemp(clang::Expr const*) /usr/local/google/home/mitchp/
llvm/clang/lib/CodeGen/CGExpr.cpp:249:10
    #28 0x5649af77f6d6 in clang::CodeGen::CodeGenFunction::EmitCallArg(clang::CodeGen::CallArgList&, clang::Expr const*, clang:
:QualType) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGCall.cpp:4742:12
    #29 0x5649af77cd28 in clang::CodeGen::CodeGenFunction::EmitCallArgs(clang::CodeGen::CallArgList&, clang::CodeGen::CodeGenFu
nction::PrototypeWrapper, llvm::iterator_range<clang::Stmt::CastIterator<clang::Expr, clang::Expr const* const, clang::Stmt con
st* const>>, clang::CodeGen::CodeGenFunction::AbstractCallee, unsigned int, clang::CodeGen::CodeGenFunction::EvaluationOrder) /
usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGCall.cpp:4587:5
    #30 0x5649af68ce0d in clang::CodeGen::CodeGenFunction::EmitCall(clang::QualType, clang::CodeGen::CGCallee const&, clang::Ca
llExpr const*, clang::CodeGen::ReturnValueSlot, llvm::Value*) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExpr.cpp:5
986:3
    #31 0x5649af689930 in clang::CodeGen::CodeGenFunction::EmitCallExpr(clang::CallExpr const*, clang::CodeGen::ReturnValueSlot
) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExpr.cpp:5476:10
    #32 0x5649af6d1c41 in (anonymous namespace)::ScalarExprEmitter::VisitCallExpr(clang::CallExpr const*) /usr/local/google/hom
e/mitchp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:597:20
    #33 0x5649af699838 in Visit /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:422:52
    #34 0x5649af699838 in clang::CodeGen::CodeGenFunction::EmitScalarExpr(clang::Expr const*, bool) /usr/local/google/home/mitc
hp/llvm/clang/lib/CodeGen/CGExprScalar.cpp:5497:8
    #35 0x5649af62ca67 in EmitAnyExpr /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGExpr.cpp:230:24
    #36 0x5649af62ca67 in clang::CodeGen::CodeGenFunction::EmitIgnoredExpr(clang::Expr const*) /usr/local/google/home/mitchp/ll
vm/clang/lib/CodeGen/CGExpr.cpp:205:18
    #37 0x5649af482cba in clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) /us
r/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGStmt.cpp:128:5
    #38 0x5649af4a7bf9 in clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt const&, bool, clang
::CodeGen::AggValueSlot) /usr/local/google/home/mitchp/llvm/clang/lib/CodeGen/CGStmt.cpp:559:7

hctim added a commit that referenced this pull request Jul 23, 2024
@hctim
Revert "[Clang][Interp] __builtin_os_log_format_buffer_size should …
5f05d5e 
…be an unevaluated builtin (#99895)"

This reverts commit 4572efe.

Reason: Introduced a memory leak that broke the sanitizer buildbots.
More information available in the original pull request
(#99895).
tbaederr added a commit that referenced this pull request Jul 23, 2024
@tbaederr
Reapply "[Clang][Interp] __builtin_os_log_format_buffer_size should…
f18dd9e 
… be an unevaluated builtin (#99895)"

This reverts commit 5f05d5e.

Reapply the original commit without the test. The memory leak is caused
by a well known problem in the new constant interpreter.
@yronglin yronglin mentioned this pull request Jul 25, 2024
Merged
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@yronglin @yuxuanchen1997
[Clang][Interp] __builtin_os_log_format_buffer_size should be an un…
54317b5 
…evaluated builtin (#99895)

Summary:
Follow the current behavior of constant evaluator,
`__builtin_os_log_format_buffer_size` should be an unevaluated builtin.

The following code is well-formed:
```
void test_builtin_os_log(void *buf, int i, const char *data) {
  constexpr int len = __builtin_os_log_format_buffer_size("%d %{public}s %{private}.16P", i, data, data);
}
```

Signed-off-by: yronglin <[email protected]>

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251336
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@hctim @yuxuanchen1997
Revert "[Clang][Interp] __builtin_os_log_format_buffer_size should …
13100d3 
…be an unevaluated builtin (#99895)"

Summary:
This reverts commit 4572efe.

Reason: Introduced a memory leak that broke the sanitizer buildbots.
More information available in the original pull request
(#99895).

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251078
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
@tbaederr @yuxuanchen1997
Reapply "[Clang][Interp] __builtin_os_log_format_buffer_size should…
7d4fe8c 
… be an unevaluated builtin (#99895)"

This reverts commit 5f05d5e.

Reapply the original commit without the test. The memory leak is caused
by a well known problem in the new constant interpreter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbaederr tbaederr tbaederr approved these changes

Assignees
No one assigned
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yronglin @llvmbot @hctim @tbaederr