Fix compiler warning about comparison of integers of different signs

[bsergean]

Here's the error:

libarchive/archive_write_disk_posix.c:3869:19: error: comparison of integers of different signs: 'unsigned long' and 'time_t' (aka 'long') [-Werror,-Wsign-compare]
        if (st->st_mtime < archive_entry_mtime(entry))
            ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
libarchive/archive_write_disk_posix.c:3872:19: error: comparison of integers of different signs: 'unsigned long' and 'time_t' (aka 'long') [-Werror,-Wsign-compare]
        if (st->st_mtime > archive_entry_mtime(entry))
            ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

st->st_mtime is an unsigned long on this platform while time_t is a long, so converting an unsigned long into a long might lose some precision, but on the other side this is the only object for which we know the type without having to know about portability.

Android seems to be the only platform where time_t is an unsigned long.
https://android.googlesource.com/platform/prebuilts/gcc/linux-x86/host/x86_64-linux-glibc2.7-4.6/+/jb-dev/sysroot/usr/include/asm/stat.h

[jsonn]

My willingness to workaround clearly broken platforms is limited. This is one of those cases I really, really dislike.

[bsergean]

Yep ...

Maybe the -W flag can be overridden so that this error is silenced when building for Android ? I'm not sure what can be done for that problem, but in the current state Android does not build. I can easily workaround that on my local build copy of libarchive but other Android users might run into this problem.

[kientzle]

Yes, this does look like a bug in Android. The problem isn't that time_t is unsigned; that happens on other platforms. But POSIX does require that struct stat have an st_mtime field of type time_t. On Android, apparently, st_mtime is unsigned long which is different from time_t.

time_t is tough to work with because the standards don't give us much in the way of tools: There are no standard definitions for the max or min values; the standards allow it to be signed or unsigned; officially, POSIX 2004 even allows time_t to be a floating-point type, though fortunately I've never heard of a platform that actually does that. In this case, of course, we're in a position where we just know we have two time values; we know archive_entry_mtime() is a time_t but we apparently can't assume the type of st->st_mtime.

To compare them correctly, we need a common type. The best candidate is probably int64_t, which has enough range to represent any realistic time value no matter whether the type is time_t, signed, or unsigned. Of course, converting a time value to int64_t isn't just a simple cast because it might be a large unsigned value. (Of course, a time value larger than INT64_MAX isn't realistic, so can only occur in error cases.)

I think the following might help:

#define to_int64_time(t) \
   ((t) < 0 ? (int64_t)(t) : (uint64_t)(t) > (uint64_t)INT64_MAX ? INT64_MAX : (int64_t)(t))

Note this has to be a macro because we don't actually know the type of the input, only that it's numeric.

The above assumes that the input is an integer type of no more than 64 bits. If the number is less than zero, t must be a signed type, so it fits in int64_t. Otherwise, it's a nonnegative value so we can cast it to uint64_t without loss. But it could be a large unsigned value, so we have to clip it to INT64_MAX.

This should allow us to do things like:

  if (to_int64_time(st->st_mtime) < to_int64_time(archive_entry_mtime(entry))) {
    ...
  }

[kientzle]

This is still broken: if st->st_mtime is out of range, then casting will change the meaning here.

See my comments in the bug about an approach that might actually do the right thing.

[kientzle]

There's also a style error here: Please don't put a space between a cast and the value.

[bsergean]

Thanks for the detailed explanation and proper solution to the problem @kientzle
I just changed the patch request to do what you're suggesting.

[san100drine]

Btw tell me if you want the comment to be reworked/rewritten, it might not be perfectly clear.

[kientzle]

Please make sure the comment has a reference to Issue #805 on github so that people who are confused by this non-obvious logic can read the full discussion.

[bsergean]

Will do.

On Fri, Oct 14, 2016 at 11:29 PM, Tim Kientzle notifications@github.com
wrote:

Please make sure the comment has a reference to Issue #805
#805 on github so that
people who are confused by this non-obvious logic can read the full
discussion.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#805 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALTvUQAa2paWfDjtSsipLtijMQ6Rv0aJks5q0HLUgaJpZM4KVU-E
.