troydhanson / uthash

C macros for hash tables and more
Other
4.18k stars 926 forks source link

Pacify a compiler with strict alignment warnings. #177

Closed brooksdavis closed 5 years ago

brooksdavis commented 5 years ago

Our compiler for CHERI (http://cheri-cpu.org) has extremely strict alignment warnings and complains about HH_FROM_ELMT's cast from a char to UT_hash_handle. Cast through a (void*) to passify the warning.

Quuxplusone commented 5 years ago

Can you post a short compileable example and the compiler error message that it produces?

It seems very strange that a compiler would warn about explicitly casting from char* to foo*, and yet not warn about explicitly casting from char* to void* to foo*.

brooksdavis commented 5 years ago

Nothing at all would work if we warned on casts from void* (consider malloc). The decision to warn on casts from char* is a deviation from general practice in our compiler. We've made the warning quite strict because roundtrips through underaligned memory render pointers invalid in our architecture and it's hard to find where that happened when you get to the point of dereferencing an invalid pointer. I've tagged @arichardson who I think is the author of this warning in CTSRD-CHERI/llvm-project.

Here's an example https://gist.github.com/brooksdavis/5471b06feb7bbd1c48d665dfd14efe56.

$ clang -target cheri-unknown-freebsd -mabi=purecap -c -o /dev/null char2struct.c -Wcast-align     
char2struct.c:8:10: warning: cast from 'char * __capability' to                
      'struct s * __capability' increases required alignment from 1 to 16      
      [-Wcast-align]
        return ((struct s*)cp);
                ^~~~~~~~~~~~~
1 warning generated.
Quuxplusone commented 5 years ago

roundtrips through underaligned memory render pointers invalid in our architecture

Well, that sounds suspiciously similar to what's happening in HH_FROM_ELT... We take a pointer to an element, cast it down to char* to do some arithmetic on it, and then cast it back up to UT_hash_handle*. Do all the test cases run and pass, on your architecture, after this change? (And what if you leave the code alone and just pass -Wno-cast-align — do they pass then? Regardless of whether you think that'd be a good long-term "fix," it'd be good to know if it works as expected.)

arichardson commented 5 years ago

I don't believe this is a warning that we have added. However, we may have changed the build system to include -Wcast-align by default. This also fails with upstream clang and can be silenced by adding the void* cast: https://godbolt.org/z/lpzNDf

brooksdavis commented 5 years ago

I'll see what I can do about running the tests. The existing test framework isn't well suited to an environment that isn't self-hosting (we run in qemu and on ~100Mhz FPGA implementations) so I'll probably only produce a subset of results.

We could certainly disable the warning, but addressing it in a single place seems easier. I know basic operations work because we can't even boot when without hitting uthash in /sbin/init.

brooksdavis commented 5 years ago

I implemented a simple do_tests.sh since we don't have perl yet (I'll submit that in a another pull request) and ran a set of tests with default flags and with -DHASH_BLOOM=16 and all of them pass.

brooksdavis commented 5 years ago

Thanks for fixing these.