Skip to content

Fix test runner constructors order and _FORTIFY_SOURCE failure - #729

Open
ArtSin wants to merge 2 commits into
canonical:mainfrom
ArtSin:fix-tests-constructors-fortify-source
Open

Fix test runner constructors order and _FORTIFY_SOURCE failure#729
ArtSin wants to merge 2 commits into
canonical:mainfrom
ArtSin:fix-tests-constructors-fortify-source

Conversation

@ArtSin

@ArtSin ArtSin commented Nov 26, 2024

Copy link
Copy Markdown

When building with clang (18.1.8 on x86_64-linux-gnu) and _FORTIFY_SOURCE=2 (or 3), I noticed that only half of the unit-test tests were run, which didn't happen with _FORTIFY_SOURCE=0. It turns out that fortified memset and _##S##_init were not optimized out, as in the case of _FORTIFY_SOURCE=0, and due to undefined order of constructors, arrays were zeroed after tests were added to them. The first commit sets the order of all functions with __attribute__((constructor)).

After fixing the first issue, tuple/decoder/type/iso8601 failed because strcpy was passed a pointer to a subarray of size 8 instead of the whole buffer. The second commit fixes this.

If the order is undefined, test arrays can be zeroed after initialization.
Pass pointer to the buffer instead of a subarray which is too small for the whole string.
@just-now

Copy link
Copy Markdown
Contributor

Hey @ArtSin, thanks for the PR!
I'll return back to it when we'll bump the version of clang.

@ArtSin

ArtSin commented Nov 29, 2024

Copy link
Copy Markdown
Author

Hey @ArtSin, thanks for the PR! I'll return back to it when we'll bump the version of clang.

The constructor order problem can be reproduced even with clang 10 in ubuntu 20.04; the _FORTIFY_SOURCE failure occurs with glibc 2.40+ (ubuntu 24.10 with -Werror disabled).

@ArtSin

ArtSin commented May 20, 2025

Copy link
Copy Markdown
Author

Ping

@marco6
marco6 force-pushed the master branch 7 times, most recently from 2ec54b4 to 3769fab Compare August 29, 2025 14:57

@marco6 marco6 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @ArtSin. Thanks for this PR.

I feel however that relying on the constructor just to zero everything is not the best way, unless I'm missing something. This is a critique to the current state of things not on your code, of course.

However I think we should take the chance and improve that instead of forcing an order on the constructors.

How about having those tests have an inline initialization? Like:

static MunitSuite _##S##_suites[TEST__CAP] = {};

And remove the constructor from TEST_SUITE__DECLARE altogether?

@marco6 marco6 added Incomplete Waiting on more information from reporter Low-priority Pull requests which can wait and are not blocking any work progress labels Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Incomplete Waiting on more information from reporter Low-priority Pull requests which can wait and are not blocking any work progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants