This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ][z/OS] Fix warnings from unsigned int to long in 32-bit mode
ClosedPublic

Authored by zibi on Oct 29 2021, 11:39 AM.

Details

Summary

This patch fixes the warnings which shows up when libcxx library started to be compiled in 32-bit mode on z/OS.
More specifically, the assignment from unsigned int to time_t aka long was flags as follows:

libcxx/include/c++/v1/__support/ibm/nanosleep.h:31:11: warning: implicit conversion changes signedness: 'unsigned int' to 'time_t' (aka 'long') [-Wsign-conversion]
  __sec = sleep(static_cast<unsigned int>(__sec));
        ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libcxx/include/c++/v1/__support/ibm/nanosleep.h:36:36: warning: implicit conversion changes signedness: 'unsigned int' to 'long' [-Wsign-conversion]
      __rem->tv_nsec = __micro_sec * 1000;
                     ~ ~~~~~~~~~~~~^~~~~~
libcxx/include/c++/v1/__support/ibm/nanosleep.h:47:36: warning: implicit conversion changes signedness: 'unsigned int' to 'long' [-Wsign-conversion]
      __rem->tv_nsec = __micro_sec * 1000;
                     ~ ~~~~~~~~~~~~^~~~~~
3 warnings generated.

Here is a small test case illustrating the issue:

typedef long    time_t ;
unsigned int sleep(unsigned int );
int main() {
  time_t sec = 0;
#ifdef FIX
  sec = static_cast<time_t>(sleep(static_cast<unsigned int>(sec)));
#else
  sec = sleep(static_cast<unsigned int>(sec));
#endif
}

clang++ -c -Wsign-conversion -m32 t.C

t.C:8:9: warning: implicit conversion changes signedness: 'unsigned int' to 'time_t' (aka 'long') [-Wsign-conversion]
  sec = sleep(static_cast<unsigned int>(sec));
      ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Diff Detail

Event Timeline

zibi requested review of this revision.Oct 29 2021, 11:39 AM
zibi created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2021, 11:39 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zibi edited the summary of this revision. (Show Details)Oct 29 2021, 11:49 AM
zibi edited reviewers, added: ldionne, muiez, DanielMcIntosh-IBM, SeanP; removed: Restricted Project.
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 29 2021, 11:49 AM
zibi added a comment.Nov 8 2021, 7:11 AM

@ldionne Can you have a look at this patch?

This is a very small patch which was already reviewed downstream.
I would like to merge this in to eliminate many warnings we are getting on z/OS.

Quuxplusone added a subscriber: Quuxplusone.

FWIW, this seems fine to me because it's limited to __support/ibm/ code.
However,
(1) In portable code (which this is not), long would be problematic because its size varies. The ranges of numbers expected here are known, because they're time units (like, "one century in microseconds" fits comfortably in int64_t but not in int32_t). So probably int32_t or int64_t would be easier to reason about. (But again, this code is non-portable by design, and I assume you know the size of long on your specific platform, and I also assume it's 64 bits.)
(2) It's odd to be changing the variable types and adding casts. For example, __sec is a time_t variable, but is invariably(?) used as an unsigned int. __micro_sec is a long variable, but is invariably used as an unsigned int. Why aren't these variables just declared as unsigned int then?

zibi added a comment.Nov 8 2021, 2:28 PM

Thank you Quuxplusone for your review and approval. Following is the rational of this patch.

This code was written for z/OS and as you indicated is not portable. The long was chosen to reduce number of cast statements. Currently we have 3 casts and with uint_32_t or int64_t we would have 5.

The root of the problem is the change of the signedness as the warning indicates and because long is a variant type on z/OS, 64-bit or 32-bit signed integer.

The type of time_t is long so as tv_nsec. The tv_sec is time_t. Therefore, they are all long.
However, both sleep() and usleep() take unsinged int` as parameter.

unsigned int sleep(unsigned int );
int usleep(useconds_t); // seconds_t is unsinged int

Mordante accepted this revision.Nov 15 2021, 9:24 AM
Mordante added a subscriber: Mordante.

LGTM!

This revision is now accepted and ready to land.Nov 15 2021, 9:24 AM
ldionne accepted this revision.Nov 15 2021, 12:47 PM