pmem / pmemfile

Userspace implementation of file APIs using persistent memory.
Other
34 stars 21 forks source link

fuse: add -fno-strict-aliasing flag #371

Closed marcinslusarz closed 7 years ago

marcinslusarz commented 7 years ago

Because of:

src/pmemfile-fuse/main.c: In function ‘pmemfile_fuse_readdir’:
src/pmemfile-fuse/main.c:146:4: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
    unsigned long nextoff = *(unsigned long *)&dirp[i];
    ^
src/pmemfile-fuse/main.c:149:4: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
    unsigned short reclen = *(unsigned short *)&dirp[i];
    ^

This change is Reviewable

GBuella commented 7 years ago

You mean *((unsigned long *)&dirp)[i]; or *(unsigned long *)(&dirp)[i]; or *(unsigned long *)(&(dirp[i])); or what exactly?

I promise I'll memorize the precedence rules once in my life, but that day of doom has not yet arrived.

marcinslusarz commented 7 years ago

You could use this as an opportunity to learn something new ;)

It's the 3rd case.

GBuella commented 7 years ago

And are you sure the compiler also interprets it as the 3rd case?

BTW, I got the same warning form GCC, but not with this diff:

diff --git a/src/pmemfile-fuse/main.c b/src/pmemfile-fuse/main.c
index af1cc8b..b107ceb 100644
--- a/src/pmemfile-fuse/main.c
+++ b/src/pmemfile-fuse/main.c
@@ -143,10 +143,10 @@ pmemfile_fuse_readdir(const char *path, void *buff, fuse_fill_dir_t fill,
                for (unsigned i = 0; i < (unsigned)r; ) {
                        i += 8;

-                       unsigned long nextoff = *(unsigned long *)&dirp[i];
+                       unsigned long nextoff = *((unsigned long *)(dirp + i));
                        i += 8;

-                       unsigned short reclen = *(unsigned short *)&dirp[i];
+                       unsigned short reclen = *((unsigned short *)(dirp + i));
                        i += 2;

                        i += 1;

Update: It looks like it really compiles to the same assembly with or without this patch. Still, that *(unsigned long *)&dirp[i]; form looks like torture. It might be readable for you, but not everyone...

marcinslusarz commented 7 years ago

Yes. [] has a higher precedence than "&" and casting. http://en.cppreference.com/w/c/language/operator_precedence

I don't know why GCC doesn't warn for your new code. It's probably a bug in GCC.

codecov[bot] commented 7 years ago

Codecov Report

Merging #371 into master will increase coverage by 0.02%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   76.94%   76.96%   +0.02%     
==========================================
  Files          67       67              
  Lines        8228     8228              
  Branches     1649     1649              
==========================================
+ Hits         6331     6333       +2     
  Misses       1488     1488              
+ Partials      409      407       -2
Flag Coverage Δ
#ltp_tests 46.53% <ø> (-0.04%) :arrow_down:
#sqlite_tests 45.34% <ø> (+0.01%) :arrow_up:
#tests_antool 14.77% <ø> (ø) :arrow_up:
#tests_posix_multi_threaded 25.59% <ø> (-0.06%) :arrow_down:
#tests_posix_single_threaded 54.26% <ø> (ø) :arrow_up:
#tests_preload 44.94% <ø> (+0.08%) :arrow_up:
Impacted Files Coverage Δ
src/libpmemfile-posix/file.c 74.86% <0%> (-0.28%) :arrow_down:
src/libpmemfile-posix/dir.c 85.25% <0%> (+0.24%) :arrow_up:
src/libpmemfile-posix/inode.c 88.65% <0%> (+0.34%) :arrow_up:
src/libpmemfile-posix/data.c 94.42% <0%> (+0.39%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ad333ec...78125f8. Read the comment docs.

sarahjelinek commented 7 years ago
:lgtm:

Reviewed 1 of 1 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable