Skip to content
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

[release/6.0] Fix build with Clang 13 #63314

Merged
merged 2 commits into from
Jan 4, 2022

Conversation

janvorli
Copy link
Member

@janvorli janvorli commented Jan 3, 2022

Port fix of build using Clang 13 to release/6.0.

Customer Impact

The recently released Clang 13 is stricter in warnings and it also started to apply assumption that this pointer is always aligned to the natural alignment of the object. These break compilation of .NET 6 e.g. on Fedora 35 where Clang 13 is the one installed. This issue was reported by Redhat developers.

Testing

Local builds of release and debug versions using clang 9 and clang 13

Risk

Low, there are no functional changes and the fix has been in main branch for about a month.

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.
@janvorli janvorli added Servicing-consider Issue for next servicing release review area-Meta labels Jan 3, 2022
@janvorli janvorli added this to the 6.0.x milestone Jan 3, 2022
@janvorli janvorli requested a review from jkotas January 3, 2022 22:49
@janvorli janvorli self-assigned this Jan 3, 2022
@ghost
Copy link

ghost commented Jan 3, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Port fix of build using Clang 13 to release/6.0.

Customer Impact

The recently released Clang 13 is stricter in warnings and it also started to apply assumption that this pointer is always aligned to the natural alignment of the object. These break compilation of .NET 6 e.g. on Fedora 35 where Clang 13 is the one installed. This issue was reported by Redhat developers.

Testing

Local builds of release and debug versions using clang 9 and clang 13

Risk

Low, there are no functional changes and the fix has been in main branch for about a month.

Author: janvorli
Assignees: janvorli
Labels:

Servicing-consider, area-Meta

Milestone: 6.0.x

@janvorli
Copy link
Member Author

janvorli commented Jan 3, 2022

cc: @omajid

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We should take for consideration in 6.0.x

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 4, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.2 Jan 4, 2022
@safern
Copy link
Member

safern commented Jan 4, 2022

@karelz @wfurt I've seen the sockets test fail across multiple PRs on the release/6.0 branch, is this known?

@safern safern merged commit f86caa5 into dotnet:release/6.0 Jan 4, 2022
@wfurt
Copy link
Member

wfurt commented Jan 4, 2022

Seems all Win11. Looks like we added win11 queue without any of the follow-up changes.
Failures related TLS 1.3 probably needs #59135 and perhaps #59359

@omajid
Copy link
Member

omajid commented Jan 27, 2022

Hey, @janvorli, do you think this is worth backporting to 3.1 and 5.0 as well? I just hit this in 3.1 too, and and this smaller patch seems to be sufficent for me:

diff --git a/src/inc/corhlpr.h b/src/inc/corhlpr.h
index 5b263a5382..c512069fca 100644
--- a/src/inc/corhlpr.h
+++ b/src/inc/corhlpr.h
@@ -335,7 +335,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 
@@ -373,9 +373,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:
@@ -578,7 +578,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/jit/bitsetasshortlong.h b/src/jit/bitsetasshortlong.h
index 17e0e3a69c..0c40283ce5 100644
--- a/src/jit/bitsetasshortlong.h
+++ b/src/jit/bitsetasshortlong.h
@@ -346,7 +346,7 @@ public:
     {
         if (IsShort(env))
         {
-            (size_t&)out = (size_t)out & ((size_t)gen | (size_t)in);
+            out = (BitSetShortLongRep)((size_t)out & ((size_t)gen | (size_t)in));
         }
         else
         {
@@ -362,7 +362,7 @@ public:
     {
         if (IsShort(env))
         {
-            (size_t&)in = (size_t)use | ((size_t)out & ~(size_t)def);
+            in = (BitSetShortLongRep)((size_t)use | ((size_t)out & ~(size_t)def));
         }
         else
         {

@janvorli
Copy link
Member Author

@omajid so the compiler warning disabling was not needed in 3.1?
I think it would be completely safe to port to 3.1 and 5.0 as it doesn't introduce any functional changes to code compiled with older compilers than clang 13.
@jeffschwMSFT what do you think?

@omajid
Copy link
Member

omajid commented Jan 27, 2022

I just built a fresh clone of coreclr 3.1 with only that smaller patch and it seems to build (I didn't run any tests, though). Git history shows that we turned off -Werror in release/3.1 with dotnet/coreclr#28026 (and 6.0 with #61442)

@janvorli
Copy link
Member Author

The other part of the change was just to silence the warnings, so if the compiler doesn't complain, we are good.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants