This is an archive of the discontinued LLVM Phabricator instance.

Headers: Make the type of SIZE_MAX the same as size_t
ClosedPublic

Authored by dexonsmith on Apr 8 2017, 2:36 PM.

Details

Summary

size_t is usually defined as unsigned long, but on 64-bit platforms,
stdint.h currently defines SIZE_MAX using "ull" (unsigned long long).
Although this is the same width, it doesn't necessarily have the same
alignment or calling convention. It also triggers printf warnings when
using the format flag "%zu" to print SIZE_MAX.

  1. Is there a better way to get the right type for SIZE_MAX?
  2. Should we do this for ptrdiff_t as well?

Diff Detail

Event Timeline

dexonsmith created this revision.Apr 8 2017, 2:36 PM
dexonsmith updated this revision to Diff 94675.Apr 10 2017, 8:27 AM

Updated clang/test/Preprocessor/stdint.c, which I missed in the original patch.

+Hal and Richard, a couple of possible reviewers.

rsmith requested changes to this revision.Apr 20 2017, 4:26 PM

This is sadly not a correct change. The relevant requirements (C11 7.20.3/2) on these macros are:

Each instance of these macros shall be replaced by a constant expression suitable for use in #if preprocessing directives, and this expression shall have the same type as would an expression that is an object of the corresponding type converted according to the integer promotions.

The "suitable for use in #if" requirement means that you cannot use a cast, and must instead use a suitable numeric suffix.

Can we instead use __SIZE_MAX__? (And likewise for ptrdiff_t etc.)

clang/test/Preprocessor/stdint.c
1416

Do we really define __SIZE_TYPE__ to unsigned a for some target?

This revision now requires changes to proceed.Apr 20 2017, 4:26 PM

Thanks for the review. __SIZE_MAX__ makes sense to me; and better even if not for the #if requirement. I'll do that for both SIZE_MAX and PTRDIFF_MAX when I get a chance.

clang/test/Preprocessor/stdint.c
1416

This testcase has a #define int a to confirm that we get intmax_t from our token pasting instead of amax_t. unsigned a looks bogus, but I figure such a user is getting what they asked for.

Hmm... presumably, this test should pass:

$ ./bin/clang -nostdinc -isystem ./lib/clang/5.0.0/include -std=c++1z -fsyntax-only t.cpp
t.cpp:3:1: error: static_assert failed
static_assert(__is_same(intptr_t, __INTPTR_TYPE__));
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
t.cpp:4:1: error: static_assert failed
static_assert(__is_same(uintptr_t, __UINTPTR_TYPE__));
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
$ cat t.cpp 
#include <stdint.h>

static_assert(__is_same(intptr_t, __INTPTR_TYPE__));
static_assert(__is_same(uintptr_t, __UINTPTR_TYPE__));

Discovered that it fails (at least for -triple x86_64-apple-macosx10.12.0) while writing tests for an improved patch that covers SIZE_MAX, PTRDIFF_MIN/MAX, UINTPTR_MAX, and INTPTR_MAX, since I was trying:

static_assert(__is_same(__typeof__(INTPTR_MIN), intptr_t));
static_assert(__is_same(__typeof__(INTPTR_MAX), intptr_t));
static_assert(__is_same(__typeof__(UINTPTR_MAX), uintptr_t));
static_assert(__is_same(__typeof__(PTRDIFF_MIN), ptrdiff_t));
static_assert(__is_same(__typeof__(PTRDIFF_MAX), ptrdiff_t));
static_assert(__is_same(__typeof__(SIZE_MAX), size_t));

and the first three assertions all failed.

Is __INTPTR_TYPE__ correct, or is intptr_t? Is it safe to fix the one that's wrong?

dexonsmith updated this revision to Diff 96078.Apr 20 2017, 7:02 PM
dexonsmith edited edge metadata.

Here's an updated patch that uses __SIZE_MAX__ and also handles the other pointer-like integers.

However, the first three static asserts fail, because intptr_t and __INTPTR_TYPE__ are different types (same for uintptr_t and __UINTPTR_TYPE__).

It looks like __INTPTR_TYPE__ was introduced in r64495 in order to help define intptr_t, and then they diverged in r89237.

Changing intptr_t would change the mangling of symbols in libcxx, so that doesn't seem safe. Is it safe to change __INTPTR_TYPE__?

We normally use stdint.h from the host C library, rather than our own version; this file is only relevant in -ffreestanding mode. So it should be safe to change.

We normally use stdint.h from the host C library, rather than our own version; this file is only relevant in -ffreestanding mode. So it should be safe to change.

Agreed; r89237 (and nearby changes: r89221, r89224, r89226) are simply wrong. We cannot determine the correct types from the bitwidth alone; there may be multiple types with the relevant width and we must pick the right one. If we back out all of those changes and instead use the correct types as provided by __*_TYPE__, we should be able to remove the __*_WIDTH__ macros too.

dexonsmith updated this revision to Diff 96269.Apr 21 2017, 6:10 PM

Thanks Eli and Richard.

Given that, I've thrown in fixes for INTMAX_MIN, INTMAX_MAX, UINTMAX_MAX, INTMAX_C(), and UINTMAX_C(), as well as fixing the typedefs for intptr_t, uintptr_t, intmax_t, and uintmax_t.

For the static assert type tests, I've explicitly added RUN lines for all the targets tested in test/Preprocessor/stdint.c.

I left out removing __INTPTR_WIDTH__, etc., since I wondered if the CHECK lines for their values were useful on their own. We could kill those in a follow-up though.

dexonsmith added inline comments.Apr 21 2017, 6:12 PM
clang/test/Headers/stdint-typeof-MINMAX.cpp
1

I copied the -ffreestanding out of test/Preprocessor/stdint.c, but I'm wondering if it even serves a purpose here... if not, we could make the RUN lines shorter (and fit 80 columns without breaking in two).

efriedma added inline comments.Apr 21 2017, 6:38 PM
clang/test/Headers/stdint-typeof-MINMAX.cpp
1

Without -ffreestanding, you're testing the contents of /usr/include/stdint.h rather than the compiler's builtin header.

dexonsmith added inline comments.Apr 22 2017, 11:27 AM
clang/test/Headers/stdint-typeof-MINMAX.cpp
1

Ah; I found no difference locally because I have no /usr/include. -v shows me:

ignoring nonexistent directory "/usr/include"

-nostdsysteminc has the same effect as -ffreestanding... but I guess I may as well leave it as is.

This revision now requires changes to proceed.Apr 24 2017, 12:42 PM
dexonsmith accepted this revision.Apr 27 2017, 3:04 PM

Thanks for the reviews! Committed in r301593.

rsmith: I just noticed you still have a red "X" here (since Phab won't let me "close"). I think I addressed your comments, but let me know if you want me to revert until you have time to look.

rsmith accepted this revision.Apr 27 2017, 3:20 PM
This revision is now accepted and ready to land.Apr 27 2017, 3:20 PM
dexonsmith closed this revision.Apr 27 2017, 3:22 PM