From 956eec37c1173828f5e44f539bd7d1cd1a9711f7 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Mon, 3 Jan 2022 12:44:57 -0800 Subject: [PATCH 1/2] Fix clang 13 induced runtime issues (#62170) The clang 13 optimizer started to assume that "this" pointer is always properly aligned. That lead to elimination of some code that was actually needed. It also takes pointer aliasing rules more strictly in one place in jit. That caused the optimizer to falsely assume that a callee with an argument passed by reference is not modifying that argument and used a stale copy of the original value at the caller site. This change fixes both of the issues. With this fix, runtime compiled using clang 13 seems to be fully functional. --- src/coreclr/inc/corhlpr.h | 8 ++++---- src/coreclr/jit/bitsetasshortlong.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/inc/corhlpr.h b/src/coreclr/inc/corhlpr.h index 450514da95c18..427e8cdc0ff5c 100644 --- a/src/coreclr/inc/corhlpr.h +++ b/src/coreclr/inc/corhlpr.h @@ -336,7 +336,7 @@ struct COR_ILMETHOD_SECT const COR_ILMETHOD_SECT* Next() const { if (!More()) return(0); - return ((COR_ILMETHOD_SECT*)(((BYTE *)this) + DataSize()))->Align(); + return ((COR_ILMETHOD_SECT*)Align(((BYTE *)this) + DataSize())); } const BYTE* Data() const @@ -374,9 +374,9 @@ struct COR_ILMETHOD_SECT return((AsSmall()->Kind & CorILMethod_Sect_FatFormat) != 0); } - const COR_ILMETHOD_SECT* Align() const + static const void* Align(const void* p) { - return((COR_ILMETHOD_SECT*) ((((UINT_PTR) this) + 3) & ~3)); + return((void*) ((((UINT_PTR) p) + 3) & ~3)); } protected: @@ -579,7 +579,7 @@ typedef struct tagCOR_ILMETHOD_FAT : IMAGE_COR_ILMETHOD_FAT const COR_ILMETHOD_SECT* GetSect() const { if (!More()) return (0); - return(((COR_ILMETHOD_SECT*) (GetCode() + GetCodeSize()))->Align()); + return(((COR_ILMETHOD_SECT*) COR_ILMETHOD_SECT::Align(GetCode() + GetCodeSize()))); } } COR_ILMETHOD_FAT; diff --git a/src/coreclr/jit/bitsetasshortlong.h b/src/coreclr/jit/bitsetasshortlong.h index dce54d6a5ca3a..365cf346a10ac 100644 --- a/src/coreclr/jit/bitsetasshortlong.h +++ b/src/coreclr/jit/bitsetasshortlong.h @@ -345,7 +345,7 @@ class BitSetOps Date: Mon, 3 Jan 2022 14:32:48 -0800 Subject: [PATCH 2/2] Fix build with clang 13 (#60328) --- eng/native/configurecompiler.cmake | 11 ++++++---- src/coreclr/jit/inlinepolicy.h | 2 +- src/libraries/Native/Unix/CMakeLists.txt | 3 +++ .../Native/Unix/System.Native/pal_process.c | 20 ++++++++++++++++++- 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/eng/native/configurecompiler.cmake b/eng/native/configurecompiler.cmake index c22469f567b72..502d432f17bb3 100644 --- a/eng/native/configurecompiler.cmake +++ b/eng/native/configurecompiler.cmake @@ -340,15 +340,17 @@ if (CLR_CMAKE_HOST_UNIX) #These seem to indicate real issues add_compile_options($<$:-Wno-invalid-offsetof>) - if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wno-unused-but-set-variable) + + if (CMAKE_C_COMPILER_ID MATCHES "Clang") + add_compile_options(-Wno-unknown-warning-option) + # The -ferror-limit is helpful during the porting, it makes sure the compiler doesn't stop # after hitting just about 20 errors. add_compile_options(-ferror-limit=4096) # Disabled warnings add_compile_options(-Wno-unused-private-field) - # Explicit constructor calls are not supported by clang (this->ClassName::ClassName()) - add_compile_options(-Wno-microsoft) # There are constants of type BOOL used in a condition. But BOOL is defined as int # and so the compiler thinks that there is a mistake. add_compile_options(-Wno-constant-logical-operand) @@ -363,8 +365,9 @@ if (CLR_CMAKE_HOST_UNIX) # to a struct or a class that has virtual members or a base class. In that case, clang # may not generate the same object layout as MSVC. add_compile_options(-Wno-incompatible-ms-struct) + + add_compile_options(-Wno-reserved-identifier) else() - add_compile_options(-Wno-unused-but-set-variable) add_compile_options(-Wno-unknown-pragmas) add_compile_options(-Wno-uninitialized) add_compile_options(-Wno-strict-aliasing) diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h index 466e17fe0e112..e604fd57a36ed 100644 --- a/src/coreclr/jit/inlinepolicy.h +++ b/src/coreclr/jit/inlinepolicy.h @@ -185,7 +185,7 @@ class DefaultPolicy : public LegalPolicy class ExtendedDefaultPolicy : public DefaultPolicy { public: - ExtendedDefaultPolicy::ExtendedDefaultPolicy(Compiler* compiler, bool isPrejitRoot) + ExtendedDefaultPolicy(Compiler* compiler, bool isPrejitRoot) : DefaultPolicy(compiler, isPrejitRoot) , m_ProfileFrequency(0.0) , m_BinaryExprWithCns(0) diff --git a/src/libraries/Native/Unix/CMakeLists.txt b/src/libraries/Native/Unix/CMakeLists.txt index 31cea57a62938..6fae902369c0e 100644 --- a/src/libraries/Native/Unix/CMakeLists.txt +++ b/src/libraries/Native/Unix/CMakeLists.txt @@ -36,6 +36,8 @@ add_compile_options(-Wno-cast-align) add_compile_options(-Wno-typedef-redefinition) add_compile_options(-Wno-c11-extensions) add_compile_options(-Wno-unknown-pragmas) +add_compile_options(-Wno-unknown-warning-option) +add_compile_options(-Wno-unused-but-set-variable) check_c_compiler_flag(-Wimplicit-fallthrough COMPILER_SUPPORTS_W_IMPLICIT_FALLTHROUGH) if (COMPILER_SUPPORTS_W_IMPLICIT_FALLTHROUGH) @@ -48,6 +50,7 @@ add_compile_options(-g) if(CMAKE_C_COMPILER_ID STREQUAL Clang) add_compile_options(-Wthread-safety) add_compile_options(-Wno-thread-safety-analysis) + add_compile_options(-Wno-reserved-identifier) elseif(CMAKE_C_COMPILER_ID STREQUAL GNU) add_compile_options(-Wno-stringop-truncation) endif() diff --git a/src/libraries/Native/Unix/System.Native/pal_process.c b/src/libraries/Native/Unix/System.Native/pal_process.c index 5e97d958f74d1..fabdfae76187c 100644 --- a/src/libraries/Native/Unix/System.Native/pal_process.c +++ b/src/libraries/Native/Unix/System.Native/pal_process.c @@ -191,6 +191,24 @@ static int SetGroups(uint32_t* userGroups, int32_t userGroupsLength, uint32_t* p return rv; } +typedef void (*VoidIntFn)(int); + +static +VoidIntFn +handler_from_sigaction (struct sigaction *sa) +{ + if (((unsigned int)sa->sa_flags) & SA_SIGINFO) + { + // work around -Wcast-function-type + void (*tmp)(void) = (void (*)(void))sa->sa_sigaction; + return (void (*)(int))tmp; + } + else + { + return sa->sa_handler; + } +} + int32_t SystemNative_ForkAndExecProcess(const char* filename, char* const argv[], char* const envp[], @@ -371,7 +389,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, } if (!sigaction(sig, NULL, &sa_old)) { - void (*oldhandler)(int) = (((unsigned int)sa_old.sa_flags) & SA_SIGINFO) ? (void (*)(int))sa_old.sa_sigaction : sa_old.sa_handler; + void (*oldhandler)(int) = handler_from_sigaction (&sa_old); if (oldhandler != SIG_IGN && oldhandler != SIG_DFL) { // It has a custom handler, put the default handler back.