Fix include guard and properly order __deregister_frame_info.

[saugustine]

Summary

This patch fixes two problems with the crtbegin.c as written:

1. In do_init, register_frame_info is not guarded by a #define, but in do_fini, deregister_frame_info is guarded by #ifndef CRT_HAS_INITFINI_ARRAY. Thus when CRT_HAS_INITFINI_ARRAY is not defined, frames are registered but then never deregistered.
   
   The frame registry mechanism builds a linked-list from the .so's static variable do_init.object, and when the .so is unloaded, this memory becomes invalid and should be deregistered.
   
   Further, libgcc's crtbegin treats the frame registry as independent from the initfini array mechanism.
   
   This patch fixes this by adding a new #define, "EH_USE_FRAME_INFO_REGISTRY", which is set by the cmake option COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY

2. Currently, do_init calls register_frame_info, and then calls the binary's constructors. This allows constructors to safely use libunwind. However, do_fini calls deregister_frame_info and then calls the binary's destructors. This prevents destructors from safely using libunwind.
   
   This patch also switches that ordering, so that destructors can safely use libunwind. As it happens, this is a fairly common scenario for thread sanitizer.

[phosek]

Thanks for these fixes! Personally, I'd be fine making a hard transition and making EH_USE_FRAME_REGISTRY independent from CRT_HAS_INITFINI_ARRAY, at least in our case it shouldn't cause any issue and I'd be surprised if anyone relied on the current behavior which is clearly a bug and not something I did intentionally.

Regarding the test, I think it'd be still useful to include it, but the dependency on libunwind might be a problem for actually running it, we might need to include a special check to only run that test when libunwind is being available.

[saugustine]

• Address upstream comments. Move backward compatibility handling to CMakeLists.

[saugustine]

• Make USE_FRAME_REGISTRY completely independent of CRT_HAS_INITFINI_ARRAY,

[saugustine]

In D70034#1739675, @phosek wrote:

Thanks for these fixes! Personally, I'd be fine making a hard transition and making EH_USE_FRAME_REGISTRY independent from CRT_HAS_INITFINI_ARRAY, at least in our case it shouldn't cause any issue and I'd be surprised if anyone relied on the current behavior which is clearly a bug and not something I did intentionally.

Regarding the test, I think it'd be still useful to include it, but the dependency on libunwind might be a problem for actually running it, we might need to include a special check to only run that test when libunwind is being available.

I figured out a more clever way on the test. It now just checks that register/deregister_frame_info is called at the proper time. That avoids the problem altogether.

EH_USE_FRAME_INFO is now completely separate from CRT_HAS_INITFINI_ARRAY, and on by default, as the original code always registered frames.

Thanks for the quick review. Will commit after running a few of the complicated cases that got me here, but I think this is right.

[MaskRay]

compiler-rt/test/crt/ctor_dtor.c

17

The LLVM coding standard uses const void * instead of const void*.

[MaskRay]

This patch fixes this by adding a new #define, "USING_FRAME_INFO_REGISTRY", which is set by the top-level define "USE_FRAME_INFO_REGISTRY". The indirection allows combining the first test with the backward compatibility check below.

I can't find USING_FRAME_INFO_REGISTRY.

[saugustine]

In the original code, register_frame_info was always called. But in the original version of this patch, I awkwardly tied it to CRT_USE_INITFINI_ARRAY, which would make it uncalled in certain circumstances. Also, we agreed that tying the two things together didn't make a lot of sense.

This revision restores the original behavior, and unties the two. It also calls __unregister_frame_info unconditionally.

[saugustine]

• Fix space.

[MaskRay]

LG aside from a request for a description update.

Now that USE_FRAME_INFO_REGISTRY no longer exists (neither does USING_FRAME_INFO_REGISTRY), the summary should be updated.

[MaskRay]

Fix include guard

Maybe the title should be updated as well. The patch does not touch any #include directive.

[saugustine]

• Make using the eh_frame registry a configure-time option.

[MaskRay]

compiler-rt/lib/crt/crtbegin.c

17

Place declarations in the scope of #ifdef EH_USE_FRAME_REGISTRY

[saugustine]

...and finally, after some off line feedback, someone pointed out to me that we really do want using the frame registry to be configurable. Not the least of which because llvm-libunwind does nothing inside __register_frame_info.

Assuming no objections, will commit later today, as nothing functional has changed since acceptance.

[saugustine]

Move prototypes for the frame info functions as requested.

[phosek]

LGTM

[MaskRay]

Closed by rG38c356176b5370164578c1d08e984964354b7189

You need Differential Revision: https://reviews.llvm.org/D70034 in the commit message so that Phabricator can close the revision automatically for you:)

compiler-rt/CMakeLists.txt

 
 include(base-config-ix)
 include(CompilerRTUtils)
 
 option(COMPILER_RT_BUILD_BUILTINS "Build builtins" ON)
 mark_as_advanced(COMPILER_RT_BUILD_BUILTINS)
 option(COMPILER_RT_BUILD_CRT "Build crtbegin.o/crtend.o" ON)
 mark_as_advanced(COMPILER_RT_BUILD_CRT)
+option(COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY "Use eh_frame in crtbegin.o/crtend.o" ON)
+mark_as_advanced(COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY)
 option(COMPILER_RT_BUILD_SANITIZERS "Build sanitizers" ON)
 mark_as_advanced(COMPILER_RT_BUILD_SANITIZERS)
 option(COMPILER_RT_BUILD_XRAY "Build xray" ON)
 mark_as_advanced(COMPILER_RT_BUILD_XRAY)
 option(COMPILER_RT_BUILD_LIBFUZZER "Build libFuzzer" ON)
 mark_as_advanced(COMPILER_RT_BUILD_LIBFUZZER)
 option(COMPILER_RT_BUILD_PROFILE "Build profile runtime" ON)
 mark_as_advanced(COMPILER_RT_BUILD_PROFILE)

compiler-rt/lib/crt/CMakeLists.txt

function(check_cxx_section_exists section output)
   file(REMOVE_RECURSE ${TARGET_NAME})
 endfunction()
 
 check_cxx_section_exists(".init_array" COMPILER_RT_HAS_INITFINI_ARRAY
   SOURCE "volatile int x;\n__attribute__((constructor)) void f() {x = 0;}\nint main() { return 0; }\n")
 
 append_list_if(COMPILER_RT_HAS_STD_C11_FLAG -std=c11 CRT_CFLAGS)
 append_list_if(COMPILER_RT_HAS_INITFINI_ARRAY -DCRT_HAS_INITFINI_ARRAY CRT_CFLAGS)
+append_list_if(COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY -DEH_USE_FRAME_REGISTRY CRT_CFLAGS)
 append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC CRT_CFLAGS)
 append_list_if(COMPILER_RT_HAS_WNO_PEDANTIC -Wno-pedantic CRT_CFLAGS)
 
 foreach(arch ${CRT_SUPPORTED_ARCH})
   add_compiler_rt_runtime(clang_rt.crtbegin
     OBJECT
     ARCHS ${arch}
     SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/crtbegin.c

compiler-rt/lib/crt/crtbegin.c

 //===-- crtbegin.c - Start of constructors and destructors ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <stddef.h>
 
 __attribute__((visibility("hidden"))) void *__dso_handle = &__dso_handle;
 
+#ifdef EH_USE_FRAME_REGISTRY
 __extension__ static void *__EH_FRAME_LIST__[]
     __attribute__((section(".eh_frame"), aligned(sizeof(void *)))) = {};
 
 extern void __register_frame_info(const void *, void *) __attribute__((weak));
 extern void *__deregister_frame_info(const void *) __attribute__((weak));
+#endif
 
 #ifndef CRT_HAS_INITFINI_ARRAY
 typedef void (*fp)(void);
 
 static fp __CTOR_LIST__[]
     __attribute__((section(".ctors"), aligned(sizeof(fp)))) = {(fp)-1};
 extern fp __CTOR_LIST_END__[];
 #endif
 
 extern void __cxa_finalize(void *) __attribute__((weak));
 
 static void __attribute__((used)) __do_init() {
   static _Bool __initialized;
   if (__builtin_expect(__initialized, 0))
     return;
   __initialized = 1;
 
+#ifdef EH_USE_FRAME_REGISTRY
   static struct { void *p[8]; } __object;
   if (__register_frame_info)
     __register_frame_info(__EH_FRAME_LIST__, &__object);
-
+#endif
 #ifndef CRT_HAS_INITFINI_ARRAY
   const size_t n = __CTOR_LIST_END__ - __CTOR_LIST__ - 1;
   for (size_t i = n; i >= 1; i--) __CTOR_LIST__[i]();
 #endif
 }
 
 #ifdef CRT_HAS_INITFINI_ARRAY
 __attribute__((section(".init_array"),
static void __attribute__((used)) __do_fini() {
   if (__builtin_expect(__finalized, 0))
     return;
   __finalized = 1;
 
   if (__cxa_finalize)
     __cxa_finalize(__dso_handle);
 
 #ifndef CRT_HAS_INITFINI_ARRAY
-  if (__deregister_frame_info)
-    __deregister_frame_info(__EH_FRAME_LIST__);
-
   const size_t n = __DTOR_LIST_END__ - __DTOR_LIST__ - 1;
   for (size_t i = 1; i <= n; i++) __DTOR_LIST__[i]();
 #endif
+#ifdef EH_USE_FRAME_REGISTRY
+  if (__deregister_frame_info)
+    __deregister_frame_info(__EH_FRAME_LIST__);
+#endif
 }
 
 #ifdef CRT_HAS_INITFINI_ARRAY
 __attribute__((section(".fini_array"),
                used)) static void (*__fini)(void) = __do_fini;
 #else  // CRT_HAS_INITFINI_ARRAY
 #if defined(__i386__) || defined(__x86_64__)
 __asm__(".pushsection .fini,\"ax\",@progbits\n\t"
     "call " __USER_LABEL_PREFIX__ "__do_fini\n\t"
     ".popsection");
 #elif defined(__arm__)
 __asm__(".pushsection .fini,\"ax\",%progbits\n\t"
     "bl " __USER_LABEL_PREFIX__ "__do_fini\n\t"
     ".popsection");
 #endif
 #endif  // CRT_HAS_INIT_FINI_ARRAY

compiler-rt/test/crt/ctor_dtor.c

 // RUN: %clang -fno-use-init-array -g -c %s -o %t.o
 // RUN: %clang -fno-use-init-array -g -o %t -nostdlib %crt1 %crti %crtbegin %t.o -lc %libgcc %crtend %crtn
 // RUN: %run %t 2>&1 | FileCheck %s
 
 #include <stdio.h>
 
-// CHECK:      ctor()
+// Ensure the various startup functions are called in the proper order.
+
+// CHECK: __register_frame_info()
+// CHECK-NEXT: ctor()
 // CHECK-NEXT: main()
 // CHECK-NEXT: dtor()
+// CHECK-NEXT: __deregister_frame_info()
+
+struct object;
+
+void __register_frame_info(const void *fi, struct object *obj) {
+  printf("__register_frame_info()\n");
+}
+
+void __deregister_frame_info(const void *fi) {
+  printf("__deregister_frame_info()\n");
+}
 
 void __attribute__((constructor)) ctor() {
   printf("ctor()\n");
 }
 
 void __attribute__((destructor)) dtor() {
   printf("dtor()\n");
 }
 
 int main() {
   printf("main()\n");
   return 0;
 }