LibTest: Replace EXPECT_CRASH with EXPECT_DEATH and remove CrashTest

[ayeteadoe]

When I was working on adding Windows support for CrashTest (which I did get working, as Windows does have native support for fork-like process cloning, see https://github.com/huntandhackett/process-cloning), there was discussion in ui-windows about an alternative: just retiring CrashTest in general given it was legacy from the Serenity repo and such tests (i.e. verifying behaviour of processes that exit with various signals) are not as relevant in the Ladybird context.

This also simplifies getting LibTest running on Windows as we no longer need to port CrashTest over and maintain the undocumented/internal Windows APIs required to get Crash::run() working.

With that an EXPECT_DEATH() macro was discussed, and this is my first stab at implementing it.

[ayeteadoe]

FYI @ADKaster

[ayeteadoe]

Update: Going to wait until https://github.com/LadybirdBrowser/ladybird/pull/4667 is merged so I can roll the Windows adjustments into this as well (I need the export symbol stuff for that), as it is slightly more involved to get weak symbols via DLLs to work on Windows so there is a little refactor for that. See my recent thread inui-windows for the details

[github-actions[bot]]

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

[R-Goc]

• should we use sigsetjmp instead?

Good question. setjmp is already sigsetjmp(buf, 1) on glibc. I guess we should restore the signal mask and use sigsetjmp? But we don't actually expect to be get signaled, at least I don't think. So maybe sigsetjmp(buf, 0)?

[ayeteadoe]

should we use sigsetjmp instead?

We can do that for the POSIX platforms, but Windows only has the standard lirbary set/longjmp.

is there a way for the 'windows' way to happen at link time, rather than assertion time? if not, we need a big flashy FIXME. the 'must be resolved at link time' requirement is on its face a 'security' requirement after all. I s'pose we can ifdef it out for 'real' distribution builds if there's no out.

Not sure about at link time, but this initial solution can definitely be optimized in 2 ways:

1. We can define a macro on the AK when configuring LibTest that says "I have an assertion handler override, and this is the name of the module you should look up". This prevents having to search through all the modules to find one that actually does define the symbol. If distribution doesn't ship LibTest then this change will already ifdef this code out
2. We can search for this symbol only at DLL load time, rather than every single invocation of the assertion handler

[ayeteadoe]

is there a way for the 'windows' way to happen at link time

Update: The answer is yes, I now have that working locally, so one more rebase incoming

[ADKaster]

Also would be nice to split the AK and LibTest+Tests parts into separate commits.

Feel free to use your name + email instead of the generic "The Ladybird Developers" btw.

[github-actions[bot]]

Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest master.

[ADKaster]

your latest push overrode the changes from https://github.com/LadybirdBrowser/ladybird/commit/38714fd93765a1789b966c93e975ef34c688f4b4. I s'pose that's why two people working on the same branch is not recommended for a rebase workflow :sweat_smile:

[ayeteadoe]

your latest push overrode the changes from https://github.com/LadybirdBrowser/ladybird/commit/38714fd93765a1789b966c93e975ef34c688f4b4.

😬 My bad! Yeah I did not pull down your changes first, I typically submit pull requests again the branch and then rebase those in as I find that helps keep things easier to track

[R-Goc]

your latest push overrode the changes from https://github.com/LadybirdBrowser/ladybird/commit/38714fd93765a1789b966c93e975ef34c688f4b4. I s'pose that's why two people working on the same branch is not recommended for a rebase workflow :sweat_smile:

Yeah I wonder if for during development of a PR, let's say when it's still a draft it wouldn't be better to just add commits. And only squash into atomic commits at the end.

[ADKaster]

No, I should have just pushed to a separate branch in my fork and said "pull these in pls"

[ayeteadoe]

Oh, EXPECT_DEATH as well is now not working on Windows, one sec

[ayeteadoe]

__declspec(selectany) is not as smart as [[gnu::weak]], AK still has to explicitly link with our customized ak_assertion_handler in AssertionHandler.cpp for it to actually select our symbol at link time, otherwise it just uses the default nullptr and the tests then start calling ak_trap

So I'm going to re-add back the code I had that created an explicit OBJECT target for AssertionHandler.cpp, then LibTestMain will link with it and on windows so will AK.

[ADKaster]

It linked just fine on my machine. Let's figure out why instead of linking AK to LibTest

[ADKaster]

If AK for normal executables has the custom assertion handler in it, then we've failed at making the custom assertion handler test-only

[ayeteadoe]

It linked just fine on my machine. Let's figure out why instead of linking AK to LibTest

Do you still have a working local copy? If so if you could push it up to some throw away branch and I can add your origin as a remote and pull it down and try myself to see if I can also run them successfully.

[ADKaster]

Sure, check discord. I posted my compiler/linker version as well.