-
-
Notifications
You must be signed in to change notification settings - Fork 874
GCC and Clang compatible on Windows #1056
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
base: master
Are you sure you want to change the base?
Conversation
public/client/TracyProfiler.cpp
Outdated
} | ||
|
||
#if defined(__clang__) || defined(__GNUC__) | ||
// memory size pointed to by buf variable is checked above |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wolfpld From what I see, on line 3122 in public/client/TracyProfiler.cpp
, there is if
statement that checks if buf
pointer will be able to contain new data and if not, then memory is allocated with size equal to size of data that will be copied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #889.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the problem is more complex than it seemed. At the moment only way I can think of to handle fail to memcpy
that is using standard language features is to use setjmp and longjmp from signal handling function. Below is simple program that shows how it could work
#include <string.h>
#include <stdint.h>
#include <setjmp.h>
#include <signal.h>
jmp_buf jumpBuffer;
void fun(int sig)
{
std::cout << "Signal " << sig << '\n';
longjmp(jumpBuffer, 1);
}
int main()
{
signal(SIGSEGV, fun);
uint32_t * dst = (uint32_t *)1234;
uint32_t src = 56;
if(setjmp(jumpBuffer) == 0)
{
memcpy(dst, &src, sizeof(uint32_t));
}
else
{
std::cout << "Error!\n";
}
std::cout << "Hello, World!\n";
return 0;
}
The only thing is that fun
and jumpBuffer
would need to be static inside some object or global.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracy already hooks SIGSEGV for other functionality, so that's additional complexity to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am continuiung to work on this and decided to use pipe write read method like one implemented for linux, but I didn't push anything yet as I am trying to make compilation work on GCC and Clang. I just wanted to ask if -flto
option enabled by line 47 in config.cmake
:
46 if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT EMSCRIPTEN)
47 set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON)
48 endif()
is absolutely required for compilation of profiler, as when compiling it with GCC and -flto
option there is error caused by too many sections:
.../MSYS2/mingw64/x86_64-w64-mingw32/bin/as.exe: CMakeFiles/tracy-profiler.dir/src/profiler/TracyMicroArchitecture.cpp.obj: too many sections (176497)
...\MSYS2\tmp\ccetkwcc.s: Assembler messages:
...\MSYS2\tmp\ccetkwcc.s: Fatal error: can't write 15 bytes to section .gnu.lto_.profile.4c08db2b of CMakeFiles/tracy-profiler.dir/src/profiler/TracyMicroArchitecture.cpp.obj: 'file too big'
I would just add condition check in cmake file to not enable CMAKE_INTERPROCEDURAL_OPTIMIZATION
for GNU, but I don't know if -flto
is required or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, LTO is entirely optional.
Note that LTO works both with gcc and clang on my end (Linux), so this may as well be some limitation specific to mingw gcc.
…c and clang compilers on Windows.
3a0af08
to
e98001c
Compare
@wolfpld, I finished changing source so it compiles with gcc and clang on Windows. I tested it on both compilers, test program that uses Tracy, and Tracy profiler itself compile successfully. The initial problem with safe |
cmake/config.cmake
Outdated
if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT EMSCRIPTEN) | ||
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON) | ||
# Mingw gcc on windows can't handle section count resulting during compilation of profiler/src/profiler/TracyMicroArchitecture.cpp | ||
if(NOT (MINGW OR "${CMAKE_C_COMPILER_ID}" STREQUAL "GNU" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") AND WIN32 AND NOT ("${CMAKE_C_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move WIN32
as first condition for better readability.
|
||
#ifdef _MSC_VER | ||
#if defined _MSC_VER || defined __clang__ || defined __GNUC__ | ||
// all checked compilers contain _stat64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes behavior on non-win32 platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix it.
import/src/json.hpp
Outdated
void write_compact_float(const number_float_t n, detail::input_format_t format) | ||
{ | ||
#ifdef __GNUC__ | ||
#if defined __GNUC__ || defined __clang__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
% clang -dM -E -x c /dev/null | grep __GNUC__
#define __GNUC__ 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being explicit. I can change it if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to go to each of the upstream projects with this change (that is not really needed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this would be due to having unified #if
conditions. I removed obviously not needed checks for __clang__
, only left ones that coexist with version checking of GNU
, as I don't know if there can be any differences, just to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, does that mean that if __GNUC__
version is checked then for sure clang
will not be supported for this block of code, or it still can be checked via #if (<GNUC version check>) || defined __clang__
? Because if only version check done in this way is not valid then it still should be possible to check if clang
is compiler that is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang defines a very old version of gcc version macro, which may affect what features are available. This can be problematic in rare circumstances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is #if (<GNUC version check>) || defined __clang__
method valid then or not? Or clang
version must be checked separately through for example __clang_major__
__clang_minor__
__clang_patchlevel__
macros?
profiler/src/main.cpp
Outdated
// clang does not have that problem | ||
#include <cmath> | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed here?
[18:59 wolf@oberon:~/tracy]% git grep std::pow
profiler/src/profiler/TracyView_Timeline.cpp: const auto mult = 1 + std::max( 0.0, 0.7 * std::pow( x, 1.6 ) - 0.8 * std::pow( x, 1.4 ) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, and turns out that using cmake from clang toolchain and gcc compiler created errors. So not needed, will remove.
// gcc throws error for not present std::pow function, | ||
// clang does not have that problem | ||
#include <cmath> | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct solution would be to change std::pow
to pow
and include math.h
(with no platform checks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks are made only to not include it where it is not needed. Code uses std::pow
so to change the least amount of stuff only header is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that solution is wrong and only works due to happenstance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, and turns out that using cmake from clang toolchain and gcc compiler created errors. So not needed, will remove.
profiler/src/stb_image.h
Outdated
@@ -1,4 +1,4 @@ | |||
/* stb_image - v2.29 - public domain image loader - http://nothings.org/stb | |||
/* stb_image - v2.28 - public domain image loader - http://nothings.org/stb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know. I did repulling, maybe something there, will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now i know, in the current repo there are 2 stb_image.h
files, test/stb_image.h
has 2.28
and profiler/src/stb_image.h
has 2.29
. When applying changes I just copied one file in place of the other to not do the same changes again. Now I checked that they are different. Will fix.
@wolfpld Fixed things you pointed out and some other minor things. |
@FilipNur Your PR rewrites the entire TracyProfiler.cpp file. Can you fix that? |
@mcourteaux I know and I don't know why. I will push changes when I resolve this issue. |
As I can't assess what your PR does due to this issue, make sure it's still relevant after my last PR was merged. |
@mcourteaux I see that your pull request is also about
Where the |
It's an incremental step. |
…c and clang compilers on Windows. Rebase
…r/tracy into gcc_and_clang_compatible
Fixed line endings in |
const size_t batchEndIdx = std::min( inputEntryList.size(), startIdx + (size_t)1024 ); | ||
|
||
printf( "Resolving symbols [%zu-%zu[\n", startIdx, batchEndIdx ); | ||
printf( "Resolving symbols [%zu-%zu]\n", startIdx, batchEndIdx ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there changes that are already applied on master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I saw the difference was space
vs tab
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely isn't.
@FilipNur You seem to have made a PR that includes a bunch of commits that are already on master. If you struggle with figuring out how to correct things, a very manual trick I have used once or twice, that resolves this very easily, is to generate a If you mess up any of the steps, there is always |
Thanks @mcourteaux, I cleaned up the code using your method. |
@wolfpld Could you check it? |
You are still changing way too much code for no reason, especially in external libraries, and there are comments you have not addressed properly. What's there to check? |
Made simple modifications so that project can be compiled wiith Clang and GCC on Windows