Scuffed Linux support

[JulianGmp]

Hey I actually got some preliminary Linux/Unix support ready... kind of.

• Disable windows specific code using the preprocessor and rely on the C++ standard lib for unix systems. There is currently no specifics for Unix/Linux or GCC.
• Add some more cout messages during the crash handler
• Add a header_reason string that is written at the top of the crashlog and write general information about the crash into it (e.g. Received signal 11 SIGSEGV)
• Add TestTool.cpp, a CLI tool that delibrately crashes to manually test the handler
• Updated the readme accordingly

A slight problem with all of this: Currently GCC (13.1.1 in my case) considers the backtrace feature to be experimental and I could not get it to output a stacktrace that worth anything, which makes this entire thing a bit useless. I added a note about that to the readme.

On another note: A lot of the things we want to do in this library are considered undefined behavior.
POSIX has many restrictions on what we're legally allowed to do in a signal handler and I feel like the C++ standard is even stricter on this, see the Signal handler section on cppreference.
I don't think we can really circumvent UB when opening a new file, so I'd suggest making sure users know what they're signing up for.

[JulianGmp]

Also I had a cmake file at some point while I made this, but decided to drop it because I'm not gonna bother testing it and making it compatible with Windows or other platforms.

If someone else wants to here's the file I used:

cmake_minimum_required(VERSION 3.26)

project(
	Crashlogs
	VERSION 0.1
	LANGUAGES CXX
)

# put all compiled files into ./bin
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/bin)
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_SOURCE_DIR}/bin)

# Make sure we're using C++23
set (CMAKE_CXX_STANDARD 23)
set (CMAKE_CXX_STANDARD_REQUIRED 23)

add_library(Crashlog
	crashlogs.cpp
)

if (UNIX)
	target_link_libraries(Crashlog
		# we need to explicitly link against the backtrace lib, this may change in the future
		# https://gcc.gnu.org/pipermail/gcc-bugs/2022-May/787683.html
		# Note that I have not the slightest idea whether this will work with ecosystems other than GNU.
		PRIVATE stdc++_libbacktrace
	)
endif (UNIX) # cmake syntax is just horrible...

if (WIN32)
	message(WARNING "Bro idk if this works on windows, you're on own.")
endif (WIN32)


add_executable(TestTool
	TestTool.cpp
)
target_link_libraries(TestTool
	PRIVATE Crashlog
)

[TylerGlaiel]

since this is like 3 different things it's gonna take me a bit to review it

I would suggest adding a stack overflow case to the test stuff, this is one I found that doesn't get optimized out
void stack_overflow(int x) { stack_overflow(x + 1); std::cout << x << std::endl; }

[JulianGmp]

since this is like 3 different things it's gonna take me a bit to review it

That's okay, it's 1 am I should go to sleep.

I would suggest adding a stack overflow case to the test stuff, this is one I found that doesn't get optimized out void stack_overflow(int x) { stack_overflow(x + 1); std::cout << x << std::endl; }

That's a good suggestion. I just tested this and it seemed to not call the signal handler, though my shell tells me it was a segmentation fault. I'll try to look into this tomorrow.

[TylerGlaiel]

I think a stack overflow would manifest as a segfault, its just that its kind of hard to do anything in the signal handler without any stack space (hence the hack in there that uses a thread for extra stack space)

with windows this could be bypassed by guaranteeing a certain amount of stack space (https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreadstackguarantee) but unfortunately that function needs to be called per-thread which limits its usefulness when dealing with libraries that may spawn their own threads

signal handler should probably not be building strings and should instead just write the signal id to a global variable

[TylerGlaiel]

main change requests:

• get rid of cout statements
• do no work in signal handlers due to limited stack space, instead of building header message there you should just write an int to a global variable representing which function / which signal

[TylerGlaiel]

oh also move tests into its own folder labeled "tests"

[JulianGmp]

I looked over your comments and implemented them, despite my love for string_view.

I'm also looking into solving the stack overflow topic. There seems to be a way to solve this, I'll try to test this and check whether it's POSIX or Linux specific.

SA_ONSTACK
Call the signal handler on an alternate signal stack
provided by sigaltstack(2). If an alternate stack is not
available, the default stack will be used. This flag is
meaningful only when establishing a signal handler.

https://man7.org/linux/man-pages/man2/sigaction.2.html

[TylerGlaiel]

dont know much about sigaltstack but from my brief look I would be concerned that stacktrace would fail to collect a trace

that said a brief look at backward.cpp indicates that they do use sigaltstack for that, which bypasses the need for the separate thread that the windows version uses. more experimentation is necessary. that said I do like the tests and the change to include the signal name into the crash log, if you wanna submit just those as their own pull request I can merge those in while still waiting on linux support

[friendlyanon]

Once you rebase your commits, you should only need to make this change to the CML:

-add_library(Crashlogs STATIC)
+add_library(Crashlogs STATIC crashlogs.cpp)
 if(WIN32)
-  target_sources(Crashlogs PRIVATE crashlogs.cpp)
   target_compile_definitions(Crashlogs PRIVATE _CRT_SECURE_NO_WARNINGS)
 endif()

Based on the mailing you posted and how it's GNU specific, stdc++_libbacktrace should not be linked in CMake project code. The user should be providing a toolchain file if their toolchain requires additional libraries to be linked. For testing, you can also just pass the link flag via whatever CMAKE_*_FLAGS.

The test tool should be added like this:

include(CTest)
if(BUILD_TESTING)
  add_executable(TestTool TestTool.cpp)
  target_link_libraries(TestTool PRIVATE Crashlogs)
endif()

The test tool could instead receive a path to where the executable should write the logs. There would preferably be an additional script that drives the test tool and inspects the logs. I'm fine with this not being implemented in this PR, just the test tool's code. I can make a nice CTest integration afterwards.

[JulianGmp]

Sorry for the late reply.

that said I do like the tests and the change to include the signal name into the crash log, if you wanna submit just those as their own pull request I can merge those in while still waiting on linux support

I agree, the functionality in GCC is wonky anyway, filed at #6. I haven't added the test tool to the cmake file yet, I can't get cmake to configure properly on current main.
I think we can put this PR on hiatus until the GCC implementation is stable enough.

Based on the mailing you posted and how it's GNU specific, stdc++libbacktrace should not be linked in CMake project code. The user should be providing a toolchain file if their toolchain requires additional libraries to be linked. For testing, you can also just pass the link flag via whatever CMAKE*_FLAGS.

No offense but I have to ask: for what purpose should we have cmake files if we would expect the user to manually intervene anyway.
Imo the cmake file should be "here's a target for the library, add it as a dependency and it'll compile".

[friendlyanon]

for what purpose should we have cmake files if we would expect the user to manually intervene anyway

In this case, it's a peculiarity of the GNU toolchain that this library is required to be linked, which is going to go away anyway soon based on the mailing list you linked. Putting extra code in to account for this is extra code that is going to bit rot sooner than later.

It's also not the requirement of the project that this library be linked, but the toolchain. It's a completely external requirement. CMake also makes it trivial to deal with toolchain requirements outside project code via command line variables or a toolchain file.

[TylerGlaiel]

In this case, it's a peculiarity of the GNU toolchain that this library is required to be linked, which is going to go away anyway soon based on the mailing list you linked. Putting extra code in to account for this is extra code that is going to bit rot sooner than later.

yeah the main purpose of this library is to get stack traces via std::stacktrace, so I would wait until that feature is fully supported in GCC (or supported enough that no temporary hacks are needed to get it to work).

[friendlyanon]

It's not a hack, it's a toolchain requirement. Toolchain requirements can be satisfied using command line variables (-D<var>=<value>), preloaded cache variables (-C<file>), or a toolchain file (-DCMAKE_TOOLCHAIN_FILE=<file> or the newer --toolchain <file>). The project will be none the wiser regardless, it only has to bother with describing its own build and usage requirements, CMake provides a myriad of ways to satisfy other requirements without having to touch the project code (i.e. forking the project). The GNU toolchain requires stdc++_libbacktrace today, but could require stdc++_libbacktrace_v2 tomorrow and if you absorbed this complexity into the project code, you would have to keep up with a particular toolchain unnecessarily.

Packaging software I'm always so delighted when a project just lets me deal with my toolchain's peculiarities instead of trying to "help" and failing horrendously.