Skip to content

Reapply "[compiler-rt][XRay] Make xray_interface.h C compliant" #141728

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Thyre
Copy link
Contributor

@Thyre Thyre commented May 28, 2025

Changes to initial PR (#140068):


Original description (from #140068)

The XRay interface header uses no C++ specific features aside from using the std namespace and including the C++ variant of C headers. Yet, these changes prevent using xray_interface.h in external tools relying on C for different reasons. Make this header C compliant by using C headers, removing the std namespace from std::size_t and guard extern "C".

To make sure that further changes to not break the interface accidentially, port one test from C++ to C. This requires the C23 standard to officially support the attribute syntax used in this test case.

Note that this only resolves this issue for xray_interface.h. xray_records.h is also not C compliant, but requires more work to port.

Fixes #139902

@llvmbot
Copy link
Member

llvmbot commented May 28, 2025

@llvm/pr-subscribers-xray

Author: Jan André Reuter (Thyre)

Changes

Changes to initial PR (#140068):


Original description (from #140068)

The XRay interface header uses no C++ specific features aside from using the std namespace and including the C++ variant of C headers. Yet, these changes prevent using xray_interface.h in external tools relying on C for different reasons. Make this header C compliant by using C headers, removing the std namespace from std::size_t and guard extern "C".

To make sure that further changes to not break the interface accidentially, port one test from C++ to C. This requires the C23 standard to officially support the attribute syntax used in this test case.

Note that this only resolves this issue for xray_interface.h. xray_records.h is also not C compliant, but requires more work to port.

Fixes #139902


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

2 Files Affected:

  • (modified) compiler-rt/include/xray/xray_interface.h (+15-6)
  • (renamed) compiler-rt/test/xray/TestCases/Posix/patching-unpatching.c (+13-8)
diff --git a/compiler-rt/include/xray/xray_interface.h b/compiler-rt/include/xray/xray_interface.h
index 675ea0cbc48c8..a34c025657621 100644
--- a/compiler-rt/include/xray/xray_interface.h
+++ b/compiler-rt/include/xray/xray_interface.h
@@ -1,4 +1,4 @@
-//===- xray_interface.h -----------------------------------------*- C++ -*-===//
+//===- xray_interface.h ---------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -14,20 +14,27 @@
 #ifndef XRAY_XRAY_INTERFACE_H
 #define XRAY_XRAY_INTERFACE_H
 
+#ifdef __cplusplus
 #include <cstddef>
 #include <cstdint>
+#else
+#include <stddef.h>
+#include <stdint.h>
+#endif
 
+#ifdef __cplusplus
 extern "C" {
+#endif
 
 /// Synchronize this with AsmPrinter::SledKind in LLVM.
-enum XRayEntryType {
+typedef enum {
   ENTRY = 0,
   EXIT = 1,
   TAIL = 2,
   LOG_ARGS_ENTRY = 3,
   CUSTOM_EVENT = 4,
   TYPED_EVENT = 5,
-};
+} XRayEntryType;
 
 /// Provide a function to invoke for when instrumentation points are hit. This
 /// is a user-visible control surface that overrides the default implementation.
@@ -68,7 +75,7 @@ extern int __xray_set_handler_arg1(void (*entry)(int32_t, XRayEntryType,
 extern int __xray_remove_handler_arg1();
 
 /// Provide a function to invoke when XRay encounters a custom event.
-extern int __xray_set_customevent_handler(void (*entry)(void *, std::size_t));
+extern int __xray_set_customevent_handler(void (*entry)(void *, size_t));
 
 /// This removes whatever the currently provided custom event handler is.
 /// Returns 1 on success, 0 on error.
@@ -86,12 +93,12 @@ extern int __xray_remove_typedevent_handler();
 
 extern uint16_t __xray_register_event_type(const char *event_type);
 
-enum XRayPatchingStatus {
+typedef enum {
   NOT_INITIALIZED = 0,
   SUCCESS = 1,
   ONGOING = 2,
   FAILED = 3,
-};
+} XRayPatchingStatus;
 
 /// This tells XRay to patch the instrumentation points in all currently loaded
 /// objects. See XRayPatchingStatus for possible result values.
@@ -173,6 +180,8 @@ extern int32_t __xray_pack_id(int32_t FuncId, int32_t ObjId);
 /// Calling __xray_init() more than once is safe across multiple threads.
 extern void __xray_init();
 
+#ifdef __cplusplus
 } // end extern "C"
+#endif
 
 #endif // XRAY_XRAY_INTERFACE_H
diff --git a/compiler-rt/test/xray/TestCases/Posix/patching-unpatching.cpp b/compiler-rt/test/xray/TestCases/Posix/patching-unpatching.c
similarity index 64%
rename from compiler-rt/test/xray/TestCases/Posix/patching-unpatching.cpp
rename to compiler-rt/test/xray/TestCases/Posix/patching-unpatching.c
index a8a820bbe45f2..488c55a0280be 100644
--- a/compiler-rt/test/xray/TestCases/Posix/patching-unpatching.cpp
+++ b/compiler-rt/test/xray/TestCases/Posix/patching-unpatching.c
@@ -1,21 +1,26 @@
 // Check that we can patch and un-patch on demand, and that logging gets invoked
 // appropriately.
 //
-// RUN: %clangxx_xray -fxray-instrument -std=c++11 %s -o %t
+// Do not run on powerpc64le, as linking XRay with C compiler causes linker error
+// due to std::__throw_system_error(int) being present in XRay libraries.
+// See https://github.com/llvm/llvm-project/issues/141598
+//
+// RUN: %clang_xray -fxray-instrument -std=c23 %s -o %t
 // RUN: env XRAY_OPTIONS="patch_premain=false" %run %t 2>&1 | FileCheck %s
-// RUN: %clangxx_xray -fxray-instrument -fno-xray-function-index -std=c++11 %s -o %t
+// RUN: %clang_xray -fxray-instrument -fno-xray-function-index -std=c23 %s -o %t
 // RUN: env XRAY_OPTIONS="patch_premain=false" %run %t 2>&1 | FileCheck %s
 
 // UNSUPPORTED: target-is-mips64,target-is-mips64el
+// UNSUPPORTED: target=powerpc64le-{{.*}}
 
 #include "xray/xray_interface.h"
 
-#include <cstdio>
+#include <stdio.h>
 
 bool called = false;
 
 void test_handler(int32_t fid, XRayEntryType type) {
-  printf("called: %d, type=%d\n", fid, static_cast<int32_t>(type));
+  printf("called: %d, type=%d\n", fid, (int32_t)(type));
   called = true;
 }
 
@@ -28,24 +33,24 @@ int main() {
   always_instrument();
   // CHECK: always instrumented called
   auto status = __xray_patch();
-  printf("patching status: %d\n", static_cast<int32_t>(status));
+  printf("patching status: %d\n", (int32_t)status);
   // CHECK-NEXT: patching status: 1
   always_instrument();
   // CHECK-NEXT: called: {{.*}}, type=0
   // CHECK-NEXT: always instrumented called
   // CHECK-NEXT: called: {{.*}}, type=1
   status = __xray_unpatch();
-  printf("patching status: %d\n", static_cast<int32_t>(status));
+  printf("patching status: %d\n", (int32_t)status);
   // CHECK-NEXT: patching status: 1
   always_instrument();
   // CHECK-NEXT: always instrumented called
   status = __xray_patch();
-  printf("patching status: %d\n", static_cast<int32_t>(status));
+  printf("patching status: %d\n", (int32_t)status);
   // CHECK-NEXT: patching status: 1
   __xray_remove_handler();
   always_instrument();
   // CHECK-NEXT: always instrumented called
   status = __xray_unpatch();
-  printf("patching status: %d\n", static_cast<int32_t>(status));
+  printf("patching status: %d\n", (int32_t)status);
   // CHECK-NEXT: patching status: 1
 }

@Thyre Thyre marked this pull request as draft May 28, 2025 08:31
@Thyre Thyre force-pushed the 139902-xray-interface-h-not-c-compliant branch from a70b96d to 8d6c1ab Compare May 28, 2025 08:34
@Thyre Thyre marked this pull request as ready for review May 28, 2025 08:35
The XRay interface header uses no C++ specific features aside from using
the std namespace and including the C++ variant of C headers. Yet, these
changes prevent using `xray_interface.h` in external tools relying on C
for different reasons. Make this header C compliant by using C headers,
removing the std namespace from std::size_t and guard `extern "C"`.

To make sure that further changes to not break the interface
accidentially, port one test from C++ to C. This requires the C23
standard to officially support the attribute syntax used in this test
case.

Note that this only resolves this issue for `xray_interface.h`.
`xray_records.h` is also not C compliant, but requires more work to
port.

Fixes llvm#139902

Signed-off-by: Jan André Reuter <j.reuter@fz-juelich.de>
@Thyre Thyre force-pushed the 139902-xray-interface-h-not-c-compliant branch from 8d6c1ab to 20a0363 Compare May 28, 2025 08:53
@Thyre
Copy link
Contributor Author

Thyre commented May 28, 2025

Checked on the same system used to report #141598, and the test is ignored correctly.

UNSUPPORTED: XRay-powerpc64le-linux :: TestCases/Posix/patching-unpatching.c (4098 of 4158)

@sebastiankreutzer, @MaskRay Can you take another look? Thanks!

Copy link
Contributor

@sebastiankreutzer sebastiankreutzer left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[compiler-rt][XRay] xray_interface.h not C compliant
3 participants