Build failure when platform is 32 bit, but stuct stat.st_size is 64 bit.

[blacklion]

When host platform is 32 bit (and size_t is 32 bit), but struct stat st_size field is 64 bit build fails:

/usr/bin/cc -DCMAKE_SOURCE_DIR_LENGTH=50 -DSTLINK_HAVE_SYS_MMAN_H -DSTLINK_HAVE_UNISTD_H -Iinclude -Isrc/mingw -O2 -pipe  -fstack-protector -fno-strict-aliasing -std=gnu99 -Wall -Wextra -Wshadow -D_FORTIFY_SOURCE=2 -fstrict-aliasing -Wundef -Wformat -Wformat-security -Wmissing-variable-declarations -Wshorten-64-to-32 -Wimplicit-function-declaration -Wno-string-plus-int -Wredundant-decls -fPIC -O2 -Werror -O2 -pipe  -fstack-protector -fno-strict-aliasing -MD -MT CMakeFiles/stlink-static.dir/src/common.c.o -MF CMakeFiles/stlink-static.dir/src/common.c.o.d -o CMakeFiles/stlink-static.dir/src/common.c.o   -c src/common.c
src/common.c:1034:41: error: implicit conversion loses integer precision: 'off_t' (aka 'long long') to 'size_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
    mf->base = (uint8_t*) mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
                          ~~~~       ~~~^~~~~~~
src/common.c:1040:18: error: implicit conversion loses integer precision: 'off_t' (aka 'long long') to 'size_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
    mf->len = st.st_size;
            ~ ~~~^~~~~~~
2 errors generated.

[xor-gate]

For some reason the struct stat st has st_size as type off_t but the mmap function expects size_t. On your platform they are different sizes. I'm not yet sure if a simple cast from off_t to size_t is appropriate. We need to investigate this. I quote from the manuals:

From http://man7.org/linux/man-pages/man2/mmap.2.html:

   #include <sys/mman.h>

   void *mmap(void *addr, size_t length, int prot, int flags,
              int fd, off_t offset);

From http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/stat.h.html

The <sys/stat.h> header shall define the structure of the data returned by the functions fstat(), lstat(), and stat(). The stat structure shall contain at least the following members:

off_t st_size For regular files, the file size in bytes.
For symbolic links, the length in bytes of the
pathname contained in the symbolic link.

From http://pubs.opengroup.org/onlinepubs/009695399/basedefs/sys/types.h.html:

off_t Used for file sizes.
size_t shall be an unsigned integer type.

[Nightwalker-87]

Please verify if the problem still exists in Release v1.6.0.

[blacklion]

It works with 1.6.0! Thank you!

[Nightwalker-87]

@blacklion: Which version did you originally use? Does it already work with v1.5.0? This would be good to know to allocate this ticket to the correct version in the changelog. I am trying to find the related fix for it.

[chenguokai]

@Nightwalker-87 I am afraid that this issue is not resolved in 1.6.0
I am the maintainer of stlink of MacPorts. I just received the same error report on snow leopard.
The error log can be found on https://build.macports.org/builders/ports-10.6_i386-builder/builds/10447/steps/install-port/logs/stdio

[blacklion]

Hmmm, did I do mistake in my verification… I need to check again.

[Nightwalker-87]

@chenguokai: Thx for the feedback. Reopening this then...

[chenguokai]

With an in-depth investigation, I suppose explicitly cast off_t to size_t is an acceptable solution.

What mmap does:

According to http://man7.org/linux/man-pages/man2/mmap.2.html

mmap() creates a new mapping in the virtual address space of the calling process.

Why it does not matter with a cast

Most modern 32 bit architects (x86, ARM, MIPS and RISC-V at least) are equipped with a 32 bit virtual address space (what size_t is expected to represent), where mmap cannot map a file larger than 4 GiB even in theory. If the file size is indeed larger than what size_t may hold, it cannot be done with this particular mechanism on 32 bit systems.

Also in practice, it is quite rare (if ever) for a single embedded related file to be over 4GiB. Checking the actual file size to be lower than 4GiB before the actual mmap would be enough to prevent a possible overflow.

Thus I think a cast (at least for 32 bit) here is acceptable. I would be happy to raise a pull request if possible.

[slyshykO]

I saw this issue and I also encounter with it using clang under win10.
In my opinion, we have improper mmap.h in the project. It should be used only for windows, where absent mmap system call. Also, we should use proper syscall with the right arguments.

[chenguokai]

@slyshykO I suppose what you want to say is about src/mmap.c, I did not check this file previously. Thanks for your reminder.
Quite agree with you on the mmap implementation.
A mmap syscall may help sharing physical RAM inter processes.

[slyshykO]

@chenguokai I said about stlink/mmap.h that used on all systems

[chenguokai]

@slyshykO I suppose we are talking about just the same thing, from different perspectives. The mmap.h is included no matter what system the software is built for. The corresponding mmap implementation is situated in src/mmap.c, rather than provided by libc on POSIX-compliant systems.
What I wanted to express is the fault is not situated in a duplicated mmap declaration but an duplicated implementation.

[Nightwalker-87]

@chenguokai: Thx for coming back to this. Can you propose an implementation of that and test it? We may include it with a PR then.

[chenguokai]

I am tweaking, while there are some other build issues following. Maybe a bigger patch is needed to build on 32 bit platforms.

For now, I am having issues with src/usb.c:192

[slyshykO]

In my opinion, using mmap function brings us many problems. Maybe we should stop using it.
Maybe better for all will be using functions from libc for work with files? This shouldn't impact on app speed dramatically.

[Nightwalker-87]

Yes, this is something to consider. However this discussion should go to #919 now.