jasmcaus / tau

A Micro (1k lines of code) Unit Test Framework for C/C++
MIT License
153 stars 29 forks source link

defaults and changing them for compiler errors #15

Open matu3ba opened 2 years ago

matu3ba commented 2 years ago

I prefer developing in a CI and thus I think disabling warnings as in TAU_DISABLE_DEBUG_WARNINGS is very uncool.

Aside, what warnings and errors are ignored should be the one of the first things in the README and documentation.

Ideally, a testing tool also allows simple integration of valgrind or better document how the user should invoke tests. You dont mention in the README, how tests should be run (as convention).

jasmcaus commented 2 years ago

I don't like disabling warnings, but at the time of writing the codebase, I had multiple things going on and didn't have time to nitpick at each warning/error. If you're open to doing this, by all means!

matu3ba commented 2 years ago

I think adding an option to enable all warnings, which means disable the disabling of warnings/errors should be sufficient. I will call this TAU_DONT_DISABLE_WARNINGS or we can discuss naming in the PR.

matu3ba commented 2 years ago

Waiting for #24 to prevent merge conflicts.

jasmcaus commented 2 years ago

If any warnings need to be fixed, they need to be fixed here on the original repo. If a user that uses Tau to test their project, and accidentally uses this macro to disable certain warnings, then Tau's warnings will appear along with the user's code. I'd really look to minimizing the amount of warnings we have currently (I know there are quite a bit) in the codebase.

matu3ba commented 2 years ago

I'll look what codepaths I can test with a toy project. I have everything except -Wformat and -Wcast-align cleaned up from -Weverything from clang, so this should be nice for testing the test framework.

matu3ba commented 2 years ago

In my toy project on this branch https://github.com/matu3ba/pbcd/tree/9dc0f8ae9c6463a38230610ede42095e195fb6f9 I get these warnings with clang -std=c17 -Weverything:

./runTest.sh 
In file included from test.c:2:
./helper.c:60:20: warning: cast from 'char *' to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align]
  hd->ptr_prefix = (uint32_t*)(hd->src);
                   ^~~~~~~~~~~~~~~~~~~~
./helper.c:69:20: warning: cast from 'char *' to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align]
  hd->ptr_prefix = (uint32_t*)(hd->dst);
                   ^~~~~~~~~~~~~~~~~~~~
./helper.c:7:9: warning: padding struct 'struct HelperData' with 4 bytes to align 'src' [-Wpadded]
  char* src;
        ^
In file included from test.c:3:
tau/tau/tau.h:104:32: warning: this function declaration is not a prototype [-Wstrict-prototypes]
typedef void (*tau_testsuite_t)();
                               ^
                                void
tau/tau/tau.h:158:36: warning: this function declaration is not a prototype [-Wstrict-prototypes]
static void __failIfInsideTestSuite();
                                   ^
                                    void
tau/tau/tau.h:158:13: warning: identifier '__failIfInsideTestSuite' is reserved because it starts with '__' [-Wreserved-identifier]
static void __failIfInsideTestSuite();
            ^
tau/tau/tau.h:159:37: warning: this function declaration is not a prototype [-Wstrict-prototypes]
static void __abortIfInsideTestSuite();
                                    ^
                                     void
tau/tau/tau.h:159:13: warning: identifier '__abortIfInsideTestSuite' is reserved because it starts with '__' [-Wreserved-identifier]
static void __abortIfInsideTestSuite();
            ^
tau/tau/tau.h:252:29: warning: cast from function call of type 'clock_t' (aka 'long') to non-matching type 'double' [-Wbad-function-cast]
    return TAU_CAST(double, clock()) * 1000000000 / CLOCKS_PER_SEC; // in nanoseconds
                            ^~~~~~~
tau/tau/misc.h:22:44: note: expanded from macro 'TAU_CAST'
    #define TAU_CAST(type, x)       ((type)x)
                                           ^
test.c:9:1: warning: identifier '_TAU_TEST_FUNC_foo_bar1' is reserved because it starts with '_' followed by a capital letter [-Wreserved-identifier]
TEST(foo, bar1)
^
tau/tau/tau.h:840:17: note: expanded from macro 'TEST'
    static void _TAU_TEST_FUNC_##TESTSUITE##_##TESTNAME(void);                                 \
                ^
<scratch space>:38:1: note: expanded from here
_TAU_TEST_FUNC_foo_bar1
^
test.c:29:3: warning: comparison between pointer and integer ('char *' and 'int') [-Wpointer-integer-compare]
  CHECK_EQ(hd.dst, 0xabc0); // TODO docs: how do we compare pointer addresses?
  ^~~~~~~~~~~~~~~~~~~~~~~~
tau/tau/tau.h:720:70: note: expanded from macro 'CHECK_EQ'
#define CHECK_EQ(actual, expected)      __TAUCMP__(actual, expected, ==, "", CHECK_EQ, TAU_FAIL_IF_INSIDE_TESTSUITE)
                                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tau/tau/tau.h:558:26: note: expanded from macro '__TAUCMP__'
            if(!((actual)cond(expected))) {                                                    \
                 ~~~~~~~~^   ~~~~~~~~~~
11 warnings generated.
pbcd.c:31:32: warning: cast from 'char *' to 'uint32_t *' (aka 'unsigned int *') increases required alignment from 1 to 4 [-Wcast-align]
    const uint32_t src_len = *((uint32_t*)(src));
                               ^~~~~~~~~~~~~~~~
1 warning generated.

Sidenode: How can I compare pointer addresses, ie to test pointer arithmetic? CHECK_EQ(charptr, 0x123) checks 0x123 as content of a backing string.

jasmcaus commented 2 years ago

The -Wstrict-prototypes warning can be fixed easily by populating empty function declarations func() with void as: func(void). You are welcome to work on this.

As for comparing pointer addresses, I suspect it's not a very common thing + my knowledge in C doesn't expand that much into this, so unless there's a reasonable argument/user traction, I think it's trivial.

matu3ba commented 1 year ago

As for comparing pointer addresses, I suspect it's not a very common thing + my knowledge in C doesn't expand that much into this, so unless there's a reasonable argument/user traction, I think it's trivial.

Casting pointers is a very evil thing to do, because accessing them can be UB: https://stackoverflow.com/questions/4810417/when-is-casting-between-pointer-types-not-undefined-behavior-in-c The only workaround to prevent UB on pointer casts is memcopy or using void pointers (acting similar to char pointers but note semantic differences on pointer arithmetic https://stackoverflow.com/questions/10058234/void-vs-char-pointer-arithmetic/52699313#52699313). Difference void vs char* pointers https://stackoverflow.com/questions/10058234/void-vs-char-pointer-arithmetic/52699313#52699313.

In short: Pointers are a huge footgun in C standard.

  1. The proper fix for access a pointer with increased alignment is to use a temporary with memcopy https://stackoverflow.com/questions/7059299/how-to-properly-convert-an-unsigned-char-array-into-an-uint32-t.
  2. To only compare pointers decrease alignment with char* pointer.
  3. To prune type info for generics use void* pointer. HOWEVER, you are responsible to call a function that provides or provide yourself 1. proper alignment, 2. sufficient storage and 3. if nececssary sufficient padding (ie within structs).