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

gcc's -Wcast-align warnings on ARM platform #64

Closed
t-mat opened this issue Mar 25, 2015 · 11 comments
Closed

gcc's -Wcast-align warnings on ARM platform #64

t-mat opened this issue Mar 25, 2015 · 11 comments

Comments

@t-mat
Copy link
Contributor

@t-mat t-mat commented Mar 25, 2015

I've got the following warnings when I build LZ4 on ARM platform (RaspberryPi Model B+ with 2015-02-16-raspbian-wheezy) :

../lib/lz4.c: In function ‘LZ4_slideInputBuffer’:
../lib/lz4.c:1324:33: warning: cast increases required alignment of target type [-Wcast-align]
../lib/lz4hc.c: In function ‘LZ4_compressHC_continue_generic’:
../lib/lz4hc.c:646:24: warning: cast increases required alignment of target type [-Wcast-align]

Line #1324 in lib/lz4.c is

char* LZ4_slideInputBuffer (void* LZ4_Data)
{
    LZ4_stream_t_internal* ctx = (LZ4_stream_t_internal*)LZ4_Data;
    int dictSize = LZ4_saveDict((LZ4_stream_t*)ctx, (char*)ctx->bufferStart, 64 KB); // Line#1324
    return (char*)(ctx->bufferStart + dictSize);
}

-Wcast-align is warning for alignment mismatch. Here, alignment of LZ4_stream_t_internal is 4 (sizeof(U32)), but alignment of LZ4_stream_t is 8 (sizeof(long long)).

So we should maintain structure's alignment more strictly like this :

#if defined(__GNUC__) || defined(__INTEL_COMPILER) || defined(__clang__)
#  define GCC_ATTR_ALIGNED(x) __attribute__ ((aligned (x)))
#endif

typedef struct {
    ...
} LZ4_stream_t_internal GCC_ATTR_ALIGNED(8);

Perhaps, adding union { long long hashTableU64[HASH_SIZE_U32/2]; ... }; is more generic and LZ4 way.

Build Log

$ uname -a
Linux raspberrypi 3.18.7+ #755 PREEMPT Thu Feb 12 17:14:31 GMT 2015 armv6l GNU/Linux
$ cd
$ git clone https://github.com/Cyan4973/lz4.git
$ cd lz4
$ git checkout dev
$ git rev-parse HEAD
80e71c6e8b0cbcd3b9976ded45cef1474a34b40c -> https://github.com/Cyan4973/lz4/commit/80e71c6e8b0cbcd3b9976ded45cef1474a34b40c
$ make
make[1]: Entering directory '/home/pi/lz4/programs'
cc      -I../lib  -O3 -std=c99 -Wall -Wextra -Wundef -Wshadow -Wcast-align -Wstrict-prototypes -pedantic -DLZ4_VERSION=\"r128\"  ../lib/lz4.c ../lib/lz4hc.c ../lib/lz4frame.c ../lib/xxhash.c bench.c lz4io.c lz4cli.c -o lz4
../lib/lz4.c: In function ‘LZ4_slideInputBuffer’:
../lib/lz4.c:1324:33: warning: cast increases required alignment of target type [-Wcast-align]
../lib/lz4hc.c: In function ‘LZ4_compressHC_continue_generic’:
../lib/lz4hc.c:646:24: warning: cast increases required alignment of target type [-Wcast-align]
cc      -I../lib  -O3 -std=c99 -Wall -Wextra -Wundef -Wshadow -Wcast-align -Wstrict-prototypes -pedantic -DLZ4_VERSION=\"r128\"  -DENABLE_LZ4C_LEGACY_OPTIONS ../lib/lz4.c ../lib/lz4hc.c ../lib/lz4frame.c ../lib/xxhash.c bench.c lz4io.c lz4cli.c -o lz4c
../lib/lz4.c: In function ‘LZ4_slideInputBuffer’:
../lib/lz4.c:1324:33: warning: cast increases required alignment of target type [-Wcast-align]
../lib/lz4hc.c: In function ‘LZ4_compressHC_continue_generic’:
../lib/lz4hc.c:646:24: warning: cast increases required alignment of target type [-Wcast-align]
make[1]: Leaving directory '/home/pi/lz4/programs'
@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

Thanks Takayuki.
I'll look into it.

I'm slightly surprised as I was expecting clang to already provide such kind of warning. It apparently slipped through.
https://travis-ci.org/Cyan4973/lz4/jobs/55790530

@t-mat
Copy link
Contributor Author

@t-mat t-mat commented Mar 25, 2015

I'm slightly surprised as I was expecting clang to already provide such kind of warning. It apparently slipped through.

Yeah, since it is basically Apple centric tool, I also expected the warning. But as x64 compiler, it is correct result though.

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

But as x64 compiler, it is correct result though.

Which raises one warning : clang -m32 would not generate 32-bits binaries, in contrast with expectation.

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

Wrong hypothesis : clang -m32 does indeed produce 32-bits binaries.

$ file lz4c
lz4c: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, not stripped
$ file lz4c32
lz4c32: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.24, not stripped `

So it seems only the warning -Wcast-align acts as if it was still 64-bits output.

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

I've updated the "dev" branch with a fix which, I hope, removes the warning complaints.
commit : a357f43

The only remaining issue is that I can't test it. I would like to build the ability to automate such a test within Travis CI. It would its re-appearance later on.

@t-mat
Copy link
Contributor Author

@t-mat t-mat commented Mar 25, 2015

Confirmed. void* trick works fine.

For testing, install cross toolchain. In this case, target architecture is arm-linux-gnueabihf

# On Raspberry Pi : retrieving target architecture name
$ gcc -dumpmachine
arm-linux-gnueabihf
# On Ubuntu x64 : install cross compiler
$ sudo apt-get install -y gcc-arm-linux-gnueabihf
$ arm-linux-gnueabihf-gcc -v  # Confirm cross compiler

# Build
$ cd /path/to/lz4
$ make clean
$ git checkout 80e71c6e
$ make CC=arm-linux-gnueabihf-gcc
$ file programs/lz4
programs/lz4: ELF 32-bit LSB  executable, ARM, EABI5 version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.32

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

Thanks Takayuki, it's exactly the test I was looking for.

Unfortunately, it doesn't work "as is" on my test system.
After installing gcc-arm-linux-gnueabihf, I'm getting the following error when trying to compile lz4 :

/usr/lib/gcc-cross/arm-linux-gnueabihf/4.8/../../../../arm-linux-gnueabihf/bin/ld: ne peut pas trouver crt1.o: Aucun fichier ou dossier de ce type
/usr/lib/gcc-cross/arm-linux-gnueabihf/4.8/../../../../arm-linux-gnueabihf/bin/ld: ne peut pas trouver crti.o: Aucun fichier ou dossier de ce type

Seems there are still some remaining problems with path, or some missing file/objects.
(Note : I'm using Linux Mint 17)

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 25, 2015

I could get it to work by using gcc-arm-linux-gnueabi instead.
Not sure why the difference, but for the purpose of the test, it doesn't matter much.
At least, I can now reproduce the issue faithfully.

@t-mat
Copy link
Contributor Author

@t-mat t-mat commented Mar 26, 2015

Strange 😕 Just an FYI, perhaps the following procedure will show some hints

(1) To ensure crt* for cross toolchain, see /usr/arm-linux-gnueabihf/lib/crt*. You'll see something like this:

$ ls -l /usr/arm-linux-gnueabihf/lib/crt*
-rw-r--r-- 1 root root 1584 Feb 26  2014 /usr/arm-linux-gnueabihf/lib/crt1.o
-rw-r--r-- 1 root root 1216 Feb 26  2014 /usr/arm-linux-gnueabihf/lib/crti.o
-rw-r--r-- 1 root root  876 Feb 26  2014 /usr/arm-linux-gnueabihf/lib/crtn.o

(2) Add -v -Wl,--verbose to gcc command line. -v is verbose mode switch for gcc and -Wl,--verbose is verbose mode for ld.

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 26, 2015

Good points !

$ ls -l /usr/arm-linux-gnueabihf/lib/crt*
ls: impossible d'accéder à /usr/arm-linux-gnueabihf/lib/crt*: Aucun fichier ou dossier de ce type

By contrast :

$ ls -l /usr/arm-linux-gnueabi/lib/crt*
-rw-r--r-- 1 root root 1584 févr. 25 2014 /usr/arm-linux-gnueabi/lib/crt1.o
-rw-r--r-- 1 root root 1208 févr. 25 2014 /usr/arm-linux-gnueabi/lib/crti.o
-rw-r--r-- 1 root root 868 févr. 25 2014 /usr/arm-linux-gnueabi/lib/crtn.o

So it's pretty clear the relevant files are simply missing. But why ?
Maybe installation is broken, for some reason.

@Cyan4973
Copy link
Member

@Cyan4973 Cyan4973 commented Mar 27, 2015

Merged into master

@Cyan4973 Cyan4973 closed this Mar 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants