This is an archive of the discontinued LLVM Phabricator instance.

Differential D84757

[compiler-rt] [profile] fix profile generate for mingw x86_64
ClosedPublic

Authored by SquallATF on Jul 28 2020, 7:52 AM.

Details

Reviewers
void
amccarth
mstorsjo
Commits
rG14c1b4017422: [compiler-rt] [profile] fix profile generate for mingw x86_64
Summary

mingw will use __sync_fetch_and_add for atomic, but on 64bit target the pointer will cast to long and the high 32 bits of the pointer will be lost. so mingw should use the same logic as msvc

Diff Detail

Event Timeline

SquallATF created this revision.Jul 28 2020, 7:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2020, 7:52 AM
Herald added subscribers: Restricted Project, mstorsjo, jfb and 2 others. · View Herald Transcript
SquallATF requested review of this revision.Jul 28 2020, 7:52 AM
Harbormaster completed remote builds in B66025: Diff 281235.Jul 28 2020, 8:33 AM
mstorsjo added reviewers: void, amccarth, mstorsjo.Jul 28 2020, 12:50 PM
mstorsjo added inline comments.
compiler-rt/lib/profile/CMakeLists.txt
4

I think this one needs to be lowercased to work properly in mingw cross compile setups.

compiler-rt/lib/profile/InstrProfilingPort.h
58

Can this cause warnings if _MSC_VER isn't defined? Would it be safer to change it into #if defined(_MSC_VER) && _MSC_VER < 1900?

SquallATF updated this revision to Diff 281446.Jul 28 2020, 6:32 PM

misc fix

Harbormaster completed remote builds in B66144: Diff 281446.Jul 28 2020, 6:33 PM
SquallATF edited the summary of this revision. (Show Details)Jul 28 2020, 6:35 PM
amccarth accepted this revision.Jul 29 2020, 8:50 AM

LGTM.

compiler-rt/lib/profile/InstrProfilingPort.h
58

Technically not necessary, since the preprocessor will replace any unrecognized macro name in a #if- or #elif-conditional with 0. But I believe this makes the intent clearer.

This revision is now accepted and ready to land.Jul 29 2020, 8:50 AM
This revision was landed with ongoing or failed builds.Jul 30 2020, 1:40 PM
Closed by commit rG14c1b4017422: [compiler-rt] [profile] fix profile generate for mingw x86_64 (authored by SquallATF, committed by mstorsjo). · Explain Why
This revision was automatically updated to reflect the committed changes.
mstorsjo added a commit: rG14c1b4017422: [compiler-rt] [profile] fix profile generate for mingw x86_64.
mstorsjo added a subscriber: hans.Jul 30 2020, 1:42 PM

@hans - @SquallATF wants this one backported to the 11.x release branch, which I think is reasonable - but maybe after waiting for a day or two in case there's any surprise.

hans added a comment.Jul 31 2020, 8:38 AM
In D84757#2185837, @mstorsjo wrote:

@hans - @SquallATF wants this one backported to the 11.x release branch, which I think is reasonable - but maybe after waiting for a day or two in case there's any surprise.

Thanks! I've put it on my list.

hans added a comment.Aug 5 2020, 10:53 AM
In D84757#2187544, @hans wrote:
In D84757#2185837, @mstorsjo wrote:

@hans - @SquallATF wants this one backported to the 11.x release branch, which I think is reasonable - but maybe after waiting for a day or two in case there's any surprise.

Thanks! I've put it on my list.

Pushed to 11.x as a862618aab2551540e175dff3a6a1d1d7c4b4a29

Revision Contents

PathSize
compiler-rt/
lib/
profile/
2 lines
2 lines

Diff 281446

compiler-rt/lib/profile/CMakeLists.txt

CHECK_CXX_SOURCE_COMPILES(" CHECK_CXX_SOURCE_COMPILES("
#ifdef _WIN32 #ifdef _WIN32
#include <Intrin.h> /* Workaround for PR19898. */ #include <intrin.h> /* Workaround for PR19898. */
mstorsjoUnsubmitted
Not Done
ReplyInline Actions

I think this one needs to be lowercased to work properly in mingw cross compile setups.

mstorsjo: I think this one needs to be lowercased to work properly in mingw cross compile setups.
#include <windows.h> #include <windows.h>
#endif #endif
int main() { int main() {
#ifdef _WIN32 #ifdef _WIN32
volatile LONG val = 1; volatile LONG val = 1;
MemoryBarrier(); MemoryBarrier();
InterlockedCompareExchange(&val, 0, 1); InterlockedCompareExchange(&val, 0, 1);
InterlockedIncrement(&val); InterlockedIncrement(&val);
▲ Show 20 Lines • Show All 129 Lines • Show Last 20 Lines

compiler-rt/lib/profile/InstrProfilingPort.h

Show First 20 Lines • Show All 49 Lines • ▼ Show 20 Lines
#define COMPILER_RT_GETHOSTNAME(Name, Len) ((void)(Name), (void)(Len), (-1)) #define COMPILER_RT_GETHOSTNAME(Name, Len) ((void)(Name), (void)(Len), (-1))
#else #else
#define COMPILER_RT_GETHOSTNAME(Name, Len) lprofGetHostName(Name, Len) #define COMPILER_RT_GETHOSTNAME(Name, Len) lprofGetHostName(Name, Len)
#endif #endif
#if COMPILER_RT_HAS_ATOMICS == 1 #if COMPILER_RT_HAS_ATOMICS == 1
#ifdef _WIN32 #ifdef _WIN32
#include <windows.h> #include <windows.h>
#if _MSC_VER < 1900 #if defined(_MSC_VER) && _MSC_VER < 1900
mstorsjoUnsubmitted
Not Done
ReplyInline Actions

Can this cause warnings if _MSC_VER isn't defined? Would it be safer to change it into #if defined(_MSC_VER) && _MSC_VER < 1900?

mstorsjo: Can this cause warnings if `_MSC_VER` isn't defined? Would it be safer to change it into `#if…
amccarthUnsubmitted
Done
ReplyInline Actions

Technically not necessary, since the preprocessor will replace any unrecognized macro name in a #if- or #elif-conditional with 0. But I believe this makes the intent clearer.

amccarth: Technically not necessary, since the preprocessor will replace any unrecognized macro name in a…
#define snprintf _snprintf #define snprintf _snprintf
#endif #endif
#if defined(_WIN64) #if defined(_WIN64)
#define COMPILER_RT_BOOL_CMPXCHG(Ptr, OldV, NewV) \ #define COMPILER_RT_BOOL_CMPXCHG(Ptr, OldV, NewV) \
(InterlockedCompareExchange64((LONGLONG volatile *)Ptr, (LONGLONG)NewV, \ (InterlockedCompareExchange64((LONGLONG volatile *)Ptr, (LONGLONG)NewV, \
(LONGLONG)OldV) == (LONGLONG)OldV) (LONGLONG)OldV) == (LONGLONG)OldV)
#define COMPILER_RT_PTR_FETCH_ADD(DomType, PtrVar, PtrIncr) \ #define COMPILER_RT_PTR_FETCH_ADD(DomType, PtrVar, PtrIncr) \
(DomType *)InterlockedExchangeAdd64((LONGLONG volatile *)&PtrVar, \ (DomType *)InterlockedExchangeAdd64((LONGLONG volatile *)&PtrVar, \
▲ Show 20 Lines • Show All 78 Lines • Show Last 20 Lines