This is an archive of the discontinued LLVM Phabricator instance.

Add support for __builtin_os_log_format[_buffer_size]
ClosedPublic

Authored by mehdi_amini on Oct 21 2016, 4:48 PM.

Details

Summary

These new builtins support a mechanism for logging OS events, using a
printf-like format string to specify the layout of data in a buffer.
The _buffer_size version of the builtin can be used to determine the size
of the buffer to allocate to hold the data, and then __builtin_os_log_format
can write data into that buffer. This implements format checking to report
mismatches between the format string and the data arguments. Most of this
code was written by Chris Willmore.

Event Timeline

mehdi_amini retitled this revision from to Add support for __builtin_os_log_format[_buffer_size].
mehdi_amini updated this object.
mehdi_amini added a reviewer: bkramer.
mehdi_amini added a subscriber: cfe-commits.

You could use llvm::any_of instead of std::any_of.

clang/include/clang/Analysis/Analyses/OSLog.h
85–87

IsPublic/IsPrivate instead of magic numbers?

clang/lib/Sema/SemaChecking.cpp
3491

auto *

Address David's feedback. Thanks David!

majnemer added inline comments.Oct 21 2016, 11:05 PM
clang/include/clang/Analysis/Analyses/OSLog.h
118–130

llvm::any_of

138–141

Magic numbers.

std::any_of -> llvm::any_of and more magic number replaced with enum

typo + clang-format

Use a separate enum to described OSLogBufferLayout header flags

bkramer accepted this revision.Oct 24 2016, 5:27 AM
bkramer edited edge metadata.

This looks good from a Clang perspective. You also might want to get review from someone who knows how the os_log API is supposed to work, as I can't comment on that.

clang/test/CodeGenObjC/os_log.m
13

This is only ever compile for x86 (you have a triple in the command line). Just drop the #ifdef.

This revision is now accepted and ready to land.Oct 24 2016, 5:27 AM

Thanks!
For the API part, this is already in production ( https://github.com/apple/swift-clang/blob/stable/lib/CodeGen/CGBuiltin.cpp#L2071 ), so it should be OK!

This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Oct 24 2016, 4:02 PM

CodeGen's CMakeLists.txt needs to be updated to link with clangAnalysis now, otherwise it breaks builds that use shared libraries.
Fixed in r285037.

diff --git a/lib/CodeGen/CMakeLists.txt b/lib/CodeGen/CMakeLists.txt
index f5d5d69..9cf34f6 100644
--- a/lib/CodeGen/CMakeLists.txt
+++ b/lib/CodeGen/CMakeLists.txt
@@ -88,6 +88,7 @@ add_clang_library(clangCodeGen
 
   LINK_LIBS
   clangAST
+  clangAnalysis
   clangBasic
   clangFrontend
   clangLex

@bkramer : is it an OK dependency to add from a clang point of view? Or should I refactor the patch to avoid introducing it?

CodeGen depending on Analysis is fine with me. Any clang tool that builds an AST will have Analysis linked in anyways so there's virtually no cost to it.