Skip to content
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

Fix detect test functions #52

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tylov
Copy link
Contributor

@tylov tylov commented Jan 17, 2023

This fixes the detection of test-functions (makes it more robust).
The strides are in general not constant between each ctest struct stored in memory, so we need to search specifically for the magic number.

With TinyC https://repo.or.cz/tinycc.git on linux, the stride is mostly 14 ints between each entry, but sometimes 16 and 18. On other platforms it is always 14 ints.
It now searches 8 ints beyond the ctest struct size, which is double of extra stride sometimes observed with TinyC. Both the current and this search is UB as it accesses memory outside the storage,
but works well in practice on most platforms.

The strides are in general not constant between each ctest struct stored in memory, so we need to search specifically for the magic number.

With TinyC https://repo.or.cz/tinycc.git on linux, the stride is mostly 14 ints between each entry, but sometimes 16 and 18. On other platforms it is always 14 ints.
It now searches 8 ints beyond the ctest struct size, which is double of extra stride sometimes observed with TinyC. Both the current and this search is UB as it accesses memory outside the storage,
but works well in practice on most platforms.
@bvdberg
Copy link
Owner

bvdberg commented Jan 18, 2023

Pretty weird that a compiler would leave gaps in a dedicated section. Is there no way to make tinycc to fix this? Clang/GCC and all other compilers used so far were ok, so I would rather not change the simple current way..

…w placing main.c before, after or between test files.
@tylov
Copy link
Contributor Author

tylov commented Jan 18, 2023

This search is already "illegal", so it is unspecified how data are organized internally. The last update I did is quite clean and short: it jumps with one ctest struct size at the time, but if it does not see the magic number, it searches up to 8 integers further for it before it quits. Your current test already peeks memory 14 ints before and after the actual stored data, so this just searches a few more bytes outside the fixed positions when needed.

@tylov
Copy link
Contributor Author

tylov commented Jan 18, 2023

If there was another magic number at the beginning of the ctest struct, you could limit how far out from the stored data you were looking when searching forward. It could still crash on "illegal memory access" in theory, but probably minimize the danger for it. So far it looks safe to peek memory around 80 bytes outside the stored table, and it is acceptable to use this trick in a development tool.

…uter ctest entry bounds - unless compiler is TINYC on LINUX. The current seach accesses the bytes 56-60 beyond the last entry.

On TINYC, it will search up to 44 byte outside if there are gaps between ctest struct entry.
@bvdberg
Copy link
Owner

bvdberg commented Jan 24, 2023

I have some questions/remarks about the patches:

  • why do you need padding after magic0?
  • please extract the MAGIC (BADFEED0/1) to a constant (like it was)
  • I would prefer not searching for the non-TinyCC compilers (but just checking the prev/next expected magic), since this lowers the chance of some accidental match

@tylov
Copy link
Contributor Author

tylov commented Jan 24, 2023

Thanks for coming back to this.

  • I will remove it; note the compiler will make this padding on a 64 bit platform anyways.
  • No problem
  • On non linux-tinyc compiler, it will only check the 4 bytes immediately before and after each ctest struct entry for the magic1 magic1 signatures, otherwise it stops search. This minimizes chances of accidental matches.
  • On linux-tinyc, it is necessary to search beyond the 4 bytes surrounding the ctest struct as there may be gaps between each entry, but it still only searches for magic numbers outside the ctest struct entries. Note: I found that tinyc often does not find the ctest entries when specifying the test-files after main.c, so currently I only apply this on the backward search.

The reason why support for TinyC is important is that it compiles an order of magnitude faster than gcc/clang, (no optimizations other than simple constant folding). This makes it is perfect for testing purposes. Executing
tcc *_test.c main.c -run
compiles and runs the tests in-memory (like a script) in a fraction of a second in most cases. You could in principle compile+run the full test-suite after each file save, and get immediate results if the tests themselves are fast.

On an entirely different matter. I made my own version with a GoogleTest-like macros API. CTEST is already fairly similar to gtest, and gtest is almost the defacto testing framework in C++ (along with Catch2). I can make an PR with these API changes, and map the current conflicting API as deprecated macros if you are interested. Example:

// GoogleTest API
#define ASSERT_NEAR(exp, real, tol) assert_dbl_compare("==", ..
#define ASSERT_DOUBLE_EQ(exp, real) assert_dbl_compare("==", ..
#define ASSERT_DOUBLE_NE(exp, real) assert_dbl_compare("!=", ..
#define ASSERT_FLOAT_EQ(exp, real) assert_dbl_compare("==", ..
#define ASSERT_EQ(exp, real) assert_compare("==", ..
#define ASSERT_STREQ(exp, real) assert_str("==", ..
#define CTEST_FIXTURE()  // CTEST_DATA
#define CTEST_F()  // CTEST2
...
// Non-GoogleTest API
#define ASSERT_NOT_NEAR(exp, real, tol) assert_dbl_compare("!=", ..
#define ASSERT_SUBSTR(exp, real) assert_str("=~", ..
...
// Deprecated API:
#define ASSERT_DBL_NEAR_TOL  ASSERT_NEAR
#define ASSERT_DBL_FAR_TOL   ASSERT_NOT_NEAR
#define ASSERT_DBL_NEAR  ASSERT_DOUBLE_EQ
#define ASSERT_DBL_FAR  ASSERT_DOUBLE_NE
#define ASSERT_EQUAL  ASSERT_EQ
#define ASSERT_STR  ASSERT_STREQ
...

In addition, you may consider adding the parallel set of EXPECT_... macros as well. It will probably require an extra internal parameter in order to specify if test should continue or not.

@bvdberg
Copy link
Owner

bvdberg commented Jan 26, 2023

GoogleTest, CUnit, CppUnit etc are the reason I started ctest. Why do you need 150 Mb of test framework when 600 lines of C can do almost the same. GoogleTest has some stuff like outputting XML etc, but come on, 150 Mb?!
So I dont really care if the macro's are exactly the same, since the rest of the scaffolding will also be different anyways.

You haven't pushed the fix for the compiler warnings yet?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants