diff mbox

[fixincludes] Fix macOS 10.12 <AvailabilityInternal.h> and <os/trace.h> (PR sanitizer/78267)

Message ID yddvavlkqwt.fsf@CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth Nov. 18, 2016, 10:45 a.m. UTC 
Hi Bruce, Mike,

> On Fri, Nov 11, 2016 at 3:18 AM, Mike Stump <mikestump@comcast.net> wrote:
>>
>> No objections from me.
>>
> Or me.  Thanks!

as discussed at length in the PR, the fixincludes route alone isn't
enough to get libsanitizer to build on Darwin 15: we stay with undefined
references to a function which had to fixinclude'd out due to use of the
Blocks extension.  On closer inspection, it turns out that the Darwin 16
<os/trace.h> effectively disables os_trace unless using clang >= 7.3.
So there's no point in jumping through hoops to achieve the same with a
large effort.

OTOH, the fixes in my fixincludes patch fix real incompatibilities
between Darwin headers an GCC, so we agreed to keep them even if
libsanitizer would build without.

So the current suggestion is to combine my fixincludes patch and Jack's
patch to disable <os/trace.h> use if !__BLOCKS__.

Bootstrapped without regressions on Darwin 16 by myself, and on 15 and
14 by Jack.

I guess this is ok for mainline now to restore bootstrap?

It may be that we add some more fixincludes fixes later, even if not
required for libsanitizer.

	Rainer

Comments

Bruce Korb Nov. 18, 2016, 5:22 p.m. UTC | #1 
I think restoring bootstrap is likely a good thing.

On Fri, Nov 18, 2016 at 2:45 AM, Rainer Orth
<ro@cebitec.uni-bielefeld.de> wrote:
>
> I guess this is ok for mainline now to restore bootstrap?
Mike Stump Nov. 18, 2016, 5:42 p.m. UTC | #2 
On Nov 18, 2016, at 2:45 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
> So the current suggestion is to combine my fixincludes patch and Jack's
> patch to disable <os/trace.h> use if !__BLOCKS__.

> I guess this is ok for mainline now to restore bootstrap?

I think we are down to everyone likes this, Ok.  The big question is, does this need a back port?


If you fix includes virtual members or data members of C/C++ classes, just be careful disappearing content as that can change the layout of the structure or class.
Bruce Korb Nov. 18, 2016, 7:24 p.m. UTC | #3 
On Fri, Nov 18, 2016 at 9:42 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Nov 18, 2016, at 2:45 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
>> So the current suggestion is to combine my fixincludes patch and Jack's
>> patch to disable <os/trace.h> use if !__BLOCKS__.
>
>> I guess this is ok for mainline now to restore bootstrap?
>
> I think we are down to everyone likes this, Ok.  The big question is, does this need a back port?

My thinking on that subject is that since include fixes do not directly affect
the compiler and, instead, facilitate its use on various platforms, it
is therefore
reasonably safe to back port.  Especially if adequate guards (selection tests)
are included in the fix to prevent it from taking action on the wrong files.
But I also think it is really a "steering committee" decision.

For me, I think it is safe enough.
Rainer Orth Nov. 21, 2016, 9:52 a.m. UTC | #4 
Hi Mike,

> On Nov 18, 2016, at 2:45 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
>> So the current suggestion is to combine my fixincludes patch and Jack's
>> patch to disable <os/trace.h> use if !__BLOCKS__.
>
>> I guess this is ok for mainline now to restore bootstrap?
>
> I think we are down to everyone likes this, Ok.  The big question is, does
> this need a back port?

while they are not necessary to fix the libsanitizer bootstrap failure,
they fix genuine header problems, so I think they're desirable.  My plan
has been to look into the problems discovered in 10.10 and 10.11 headers
while developing this one, get them into mainline as time permits and
backport the whole bunch afterwards.

> If you fix includes virtual members or data members of C/C++ classes, just
> be careful disappearing content as that can change the layout of the
> structure or class.

Understood.  So far, the fixes have just removed attributes not
supported by GCC, or types using such extensions and function
declarations using them.

	Rainer
Rainer Orth Nov. 21, 2016, 9:54 a.m. UTC | #5 
Hi Bruce,

> On Fri, Nov 18, 2016 at 9:42 AM, Mike Stump <mikestump@comcast.net> wrote:
>> On Nov 18, 2016, at 2:45 AM, Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE> wrote:
>>> So the current suggestion is to combine my fixincludes patch and Jack's
>>> patch to disable <os/trace.h> use if !__BLOCKS__.
>>
>>> I guess this is ok for mainline now to restore bootstrap?
>>
>> I think we are down to everyone likes this, Ok.  The big question is,
>> does this need a back port?
>
> My thinking on that subject is that since include fixes do not directly affect
> the compiler and, instead, facilitate its use on various platforms, it
> is therefore
> reasonably safe to back port.  Especially if adequate guards (selection tests)
> are included in the fix to prevent it from taking action on the wrong files.
> But I also think it is really a "steering committee" decision.
>
> For me, I think it is safe enough.

Agreed: especially for the WIP 10.10 and 10.11 fixes I've found
instances where people had stumbled across incompatibilities in contexts
other than libsanitizer.  So the fixes would improve their user
experience regardless :-)

	Rainer
diff mbox

Patch

# HG changeset patch
# Parent  14eb543b5cb5e7a2bdea7430e3bf3b771bf0e54f
Fix macOS 10.12 <AvailabilityInternal.h> and <os/trace.h> (PR sanitizer/78267)

diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def
--- a/fixincludes/inclhack.def
+++ b/fixincludes/inclhack.def
@@ -1338,6 +1338,32 @@  fix = {
 };
 
 /*
+ *  macOS 10.12 <AvailabilityInternal.h> uses __attribute__((availability))
+ *  unconditionally.
+ */
+fix = {
+    hackname  = darwin_availabilityinternal;
+    mach      = "*-*-darwin*";
+    files     = AvailabilityInternal.h;
+    select    = "#define[ \t]+(__API_[ADU]\\([^)]*\\)).*";
+    c_fix     = format;
+    c_fix_arg = <<- _EOFix_
+	#if defined(__has_attribute)
+	  #if __has_attribute(availability)
+	%0
+	  #else
+	    #define %1
+	  #endif
+	#else
+	    #define %1
+	#endif
+	_EOFix_;
+
+    test_text = "#define __API_A(x) __attribute__((availability(__API_AVAILABLE_PLATFORM_##x)))\n"
+		"#define __API_D(msg,x) __attribute__((availability(__API_DEPRECATED_PLATFORM_##x,message=msg)))";
+};
+
+/*
  *  For the AAB_darwin7_9_long_double_funcs fix to be useful,
  *  you have to not use "" includes.
  */
@@ -1410,6 +1436,62 @@  fix = {
 };
 
 /*
+ *  Mac OS X 10.11 <os/trace.h> uses attribute on function definition.
+ */
+fix = {
+  hackname  = darwin_os_trace_1;
+  mach      = "*-*-darwin*";
+  files     = os/trace.h;
+  select    = "^(_os_trace_verify_printf.*) (__attribute__.*)";
+  c_fix     = format;
+  c_fix_arg = "%1";
+  test_text = "_os_trace_verify_printf(const char *msg, ...) __attribute__((format(printf, 1, 2)))";
+};
+
+/*
+ *  Mac OS X 10.1[012] <os/trace.h> os_trace_payload_t typedef uses Blocks
+ *  extension without guard.
+ */
+fix = {
+  hackname  = darwin_os_trace_2;
+  mach      = "*-*-darwin*";
+  files     = os/trace.h;
+  select    = "typedef.*\\^os_trace_payload_t.*";
+  c_fix     = format;
+  c_fix_arg = "#if __BLOCKS__\n%0\n#endif";
+  test_text = "typedef void (^os_trace_payload_t)(xpc_object_t xdict);";
+};
+
+/*
+ *  In Mac OS X 10.1[012] <os/trace.h>, need to guard users of
+ *  os_trace_payload_t typedef, too.
+ */
+fix = {
+  hackname  = darwin_os_trace_3;
+  mach      = "*-*-darwin*";
+  files     = os/trace.h;
+  select    = <<- _EOSelect_
+	__(API|OSX)_.*
+	OS_EXPORT.*
+	.*
+	_os_trace.*os_trace_payload_t payload);
+	_EOSelect_;
+  c_fix     = format;
+  c_fix_arg = "#if __BLOCKS__\n%0\n#endif";
+  test_text = <<- _EOText_
+	__API_AVAILABLE(macosx(10.10), ios(8.0), watchos(2.0), tvos(8.0))
+	OS_EXPORT OS_NOTHROW OS_NOT_TAIL_CALLED
+	void
+	_os_trace_with_buffer(void *dso, const char *message, uint8_t type, const void *buffer, size_t buffer_size, os_trace_payload_t payload);
+
+	__OSX_AVAILABLE_STARTING(__MAC_10_12, __IPHONE_10_0)
+	OS_EXPORT OS_NOTHROW
+	void
+	_os_trace_internal(void *dso, uint8_t type, const char *format, const uint8_t *buf, size_t buf_size, os_trace_payload_t payload);
+	_EOText_;
+};
+
+/*
  *  __private_extern__ doesn't exist in FSF GCC.  Even if it did,
  *  why would you ever put it in a system header file?
  */
@@ -2638,7 +2720,6 @@  fix = {
     c-fix-arg = "#  define	UINT_%164_MAX	__UINT64_MAX__";
     test-text = "#  define       UINT_FAST64_MAX        ULLONG_MAX\n"
 		"#  define       UINT_LEAST64_MAX        ULLONG_MAX\n";
-	_EOFix_;
 };
 
 /*
diff --git a/fixincludes/tests/base/AvailabilityInternal.h b/fixincludes/tests/base/AvailabilityInternal.h
new file mode 100644
--- /dev/null
+++ b/fixincludes/tests/base/AvailabilityInternal.h
@@ -0,0 +1,31 @@ 
+/*  DO NOT EDIT THIS FILE.
+
+    It has been auto-edited by fixincludes from:
+
+	"fixinc/tests/inc/AvailabilityInternal.h"
+
+    This had to be done to correct non-standard usages in the
+    original, manufacturer supplied header file.  */
+
+
+
+#if defined( DARWIN_AVAILABILITYINTERNAL_CHECK )
+#if defined(__has_attribute)
+  #if __has_attribute(availability)
+#define __API_A(x) __attribute__((availability(__API_AVAILABLE_PLATFORM_##x)))
+  #else
+    #define __API_A(x)
+  #endif
+#else
+    #define __API_A(x)
+#endif
+#if defined(__has_attribute)
+  #if __has_attribute(availability)
+#define __API_D(msg,x) __attribute__((availability(__API_DEPRECATED_PLATFORM_##x,message=msg)))
+  #else
+    #define __API_D(msg,x)
+  #endif
+#else
+    #define __API_D(msg,x)
+#endif
+#endif  /* DARWIN_AVAILABILITYINTERNAL_CHECK */
diff --git a/fixincludes/tests/base/os/trace.h b/fixincludes/tests/base/os/trace.h
new file mode 100644
--- /dev/null
+++ b/fixincludes/tests/base/os/trace.h
@@ -0,0 +1,38 @@ 
+/*  DO NOT EDIT THIS FILE.
+
+    It has been auto-edited by fixincludes from:
+
+	"fixinc/tests/inc/os/trace.h"
+
+    This had to be done to correct non-standard usages in the
+    original, manufacturer supplied header file.  */
+
+
+
+#if defined( DARWIN_OS_TRACE_1_CHECK )
+_os_trace_verify_printf(const char *msg, ...)
+#endif  /* DARWIN_OS_TRACE_1_CHECK */
+
+
+#if defined( DARWIN_OS_TRACE_2_CHECK )
+#if __BLOCKS__
+typedef void (^os_trace_payload_t)(xpc_object_t xdict);
+#endif
+#endif  /* DARWIN_OS_TRACE_2_CHECK */
+
+
+#if defined( DARWIN_OS_TRACE_3_CHECK )
+#if __BLOCKS__
+__API_AVAILABLE(macosx(10.10), ios(8.0), watchos(2.0), tvos(8.0))
+OS_EXPORT OS_NOTHROW OS_NOT_TAIL_CALLED
+void
+_os_trace_with_buffer(void *dso, const char *message, uint8_t type, const void *buffer, size_t buffer_size, os_trace_payload_t payload);
+#endif
+
+#if __BLOCKS__
+__OSX_AVAILABLE_STARTING(__MAC_10_12, __IPHONE_10_0)
+OS_EXPORT OS_NOTHROW
+void
+_os_trace_internal(void *dso, uint8_t type, const char *format, const uint8_t *buf, size_t buf_size, os_trace_payload_t payload);
+#endif
+#endif  /* DARWIN_OS_TRACE_3_CHECK */
diff --git a/libsanitizer/sanitizer_common/sanitizer_mac.cc b/libsanitizer/sanitizer_common/sanitizer_mac.cc
--- a/libsanitizer/sanitizer_common/sanitizer_mac.cc
+++ b/libsanitizer/sanitizer_common/sanitizer_mac.cc
@@ -34,7 +34,7 @@ 
 extern char **environ;
 #endif
 
-#if defined(__has_include) && __has_include(<os/trace.h>)
+#if defined(__has_include) && __has_include(<os/trace.h>) && defined(__BLOCKS__)
 #define SANITIZER_OS_TRACE 1
 #include <os/trace.h>
 #else