WIP: Fix typo in util.c

[sergey-dryabzhinsky]

There must be mtime

Error popup if I use gcc-4.4 and build under Ubuntu 10.04.
And in arm builds up to Ubuntu 14.04.

Probably there must be more clever check for attributes of stat_t structure.

[facebook-github-bot]

Hi sergey-dryabzhinsky! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[Cyan4973]

Thanks @sergey-dryabzhinsky .

I'm surprised : this code path was supposed to be already tested.
The proposed modification changes the meaning and type of some fields, meaning that both versions can't be compatible, one of these code must be bogus.

Bottom line : it seems we are missing a test to ensure that this code path is being compiled properly. It should be visible during CI.

Also, minor thing, but it seems that this code path is only used for relatively recent POSIX platforms, compliant with version 200809L. I wonder if that's within scope of older compilers such as gcc 4.4.

[Cyan4973]

Checked : this code path is tested on CircleCI.
I'm wondering, why should it compile correctly for both versions ?

before :
timebuf[1] = statbuf->st_mtim;

after :
timebuf[1].tv_sec = statbuf->st_mtime;

It seems this story is more than just about a typo ...

[Cyan4973]

Follow up :
I found this SO thread, which seems to clarify the topic :
https://stackoverflow.com/questions/23944199/why-does-my-struct-stat-have-a-st-mtim-instead-of-st-mtime-field

Bottom line : only st_mtime is part of the official specification of struct stat. st_mtim is an implementation detail, which may or may not be present, depending on platform.

They nonetheless both work (on platform with st_mtim), because st_mtime is defined as a macro, effectively translated as st_mtim.tv_sec. That's why it felt so weird : they are not 2 different fields !

Since only st_mtime is standard, this validates your proposed change.

[felixhandte]

Something is wrong here.

And maybe the easiest thing to do is just give up on sub-second precision here, but I'd rather not, if possible (since this can lead to the destination file having an earlier mtime than the source file, which is weird).

@Cyan4973, the spec you linked is POSIX 2001. st_{a,m,c}tim are defined though in POSIX 2008, where they are not an implementation detail. They must be present. This is why this code checks that PLATFORM_POSIX_VERSION >= 200809L.

So from a theoretical perspective, I'm confident that the code using st_mtim was correct. In reality though this is the second report we've had of a platform that claims POSIX 2008 compliance, but that doesn't have st_mtim. I'm not sure what to make of that.

man 2 stat talks about support for this being added in glibc 2.12. I would think that glibcs older than that wouldn't report themselves as 2008-compliant, but maybe they do? @sergey-dryabzhinsky, can you report which glibc version you encountered this on (potentially visible via ldd --version)?

[Cyan4973]

Incomplete compliance to posix standards is unfortunately not so rare,
and while this should not happen in theory, in practice this is a pain we too often have to deal with.

A general idea when dealing with such problem :

• Complete the posix check with some additional macro check. The idea is to find a macro which, when defined, is a very strong hint that the wanted symbol should also be defined in the library, because they kind of work together. Here is an example. Sometimes it's trivial to find one, sometimes such macro is not present. Sometimes it's not enough and there are still some corner cases.
• A second option is to blacklist some systems, known to not support a wanted symbol while claiming compliance. This is least preferred, but may sometimes be the only remaining option. Black-listing older versions of glibc get into this category.
• Finally, another option is to change course and select to depend on a different symbol from a more widely available standard.

In this specific case, if a functionality of POSIX 2008 is available in POSIX 2001, using a different semantic, then it might be better to use the 2001 one, on the ground that it's more widely supported.

Yet, you mention that they are not identical, with the 2001 function being limited to second accuracy. In a benchmark function, such limitation would be a deal breaker.

But in this case, the objective is to "set access and modification times". Does it have to be sub-second ? Is this accuracy actually stored somewhere ?

[felixhandte]

• Complete the posix check with some additional macro check. The idea is to find a macro which, when defined, is a very strong hint that the wanted symbol should also be defined in the library, because they kind of work together. Here is an example. Sometimes it's trivial to find one, sometimes such macro is not present. Sometimes it's not enough and there are still some corner cases.

Good point! Maybe we can additionally gate on #if defined(st_mtime), since we are told, when st_mtim is present, st_mtime is a macro that resolves to st_mtim.tv_sec.

But in this case, the objective is to "set access and modification times". Does it have to be sub-second ? Is this accuracy actually stored somewhere ?

Linux filesystems all store nanosecond-precision timestamps. I'm not sure about mac and win, but I'm pretty sure they're sub-second as well. I think in particular the concerning case is that truncation always makes timestamps earlier, which makes a derivative file created by zstd apparently older than its source. This could be confusing to systems such as make.

[sergey-dryabzhinsky]

@felixhandte
On Ubuntu 10.04 eglibc was 2.11.1 + patches, i386 and amd64 affected.
On Ubuntu 14.04 eglibc is 2.19 + patches, armhf and arm64 affected, i386 and amd64 has no issues.

[felixhandte]

Cool. So that matches. I've put up #1920, which I believe should mitigate this issue. Feel free to try it out locally to confirm?

[sergey-dryabzhinsky]

@felixhandte New error occurs on Ubuntu 10.04:

make[3]: Entering directory `/build/libzstd-1.4.4+dfsg/programs'
cc -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Wformat-security -Werror=format-security -O2 -march=i686 -DZSTD_CLEVEL_DEFAULT=6 -DZSTD_LEGACY_MULTITHREADED_API=1 -D_FORTIFY_SOURCE=2 -DZSTD_BUILD_TESTS:BOOL=OFF -DBACKTRACE_ENABLE=0  -D_FORTIFY_SOURCE=2 -I../lib -I../lib/common -I../lib/compress -I../lib/dictBuilder -DXXH_NAMESPACE=ZSTD_ -I../lib/legacy -DZSTD_MULTITHREAD -DZSTD_GZCOMPRESS -DZSTD_GZDECOMPRESS -DZSTD_LZMACOMPRESS -DZSTD_LZMADECOMPRESS -DZSTD_LZ4COMPRESS -DZSTD_LZ4DECOMPRESS -DZSTD_LEGACY_SUPPORT=5  -c -o zstdcli.o zstdcli.c
<command-line>: warning: missing whitespace after the macro name
cc -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Wformat-security -Werror=format-security -O2 -march=i686 -DZSTD_CLEVEL_DEFAULT=6 -DZSTD_LEGACY_MULTITHREADED_API=1 -D_FORTIFY_SOURCE=2 -DZSTD_BUILD_TESTS:BOOL=OFF -DBACKTRACE_ENABLE=0  -D_FORTIFY_SOURCE=2 -I../lib -I../lib/common -I../lib/compress -I../lib/dictBuilder -DXXH_NAMESPACE=ZSTD_ -I../lib/legacy -DZSTD_MULTITHREAD -DZSTD_GZCOMPRESS -DZSTD_GZDECOMPRESS -DZSTD_LZMACOMPRESS -DZSTD_LZMADECOMPRESS -DZSTD_LZ4COMPRESS -DZSTD_LZ4DECOMPRESS -DZSTD_LEGACY_SUPPORT=5  -c -o util.o util.c
<command-line>: warning: missing whitespace after the macro name
util.c: In function 'UTIL_setFileStat':
util.c:78: error: storage size of 'timebuf' isn't known
make[3]: *** [util.o] Error 1

[sergey-dryabzhinsky]

Same at Ubuntu 14.04 on arm64.

[Cyan4973]

New error occurs on Ubuntu 10.04:
Same at Ubuntu 14.04 on arm64.

Should we consider adding some similar test in CI to experience the issue, and make sure it remains fixed in the future ?
Ubuntu 14.04 is trusty, right ?

[sergey-dryabzhinsky]

Right

[Cyan4973]

OK,
but we already do have a compilation test under trusty,
and it has been compiling fine so far ...

https://travis-ci.org/facebook/zstd/jobs/621243704#L10

So maybe there are additional conditions required

note : this test compiles ppc binaries, although from a cross-compiler environment

[felixhandte]

As reported, it only fails on Trusty with arm, x86 succeeds. I'll add such a build. First I have to fix make armtest, which is broken.