[PATCH] D144218: [Clang] [AVR] Fix USHRT_MAX for 16-bit int.

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Feb 24 05:10:16 PST 2023


aaron.ballman added reviewers: jyknight, clang-language-wg.
aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:43-45
+- The definition of ``USHRT_MAX`` in the freestanding ``<limits.h>`` no longer
+  overflows on AVR (where ``sizeof(unsigned int) == sizeof(unsigned short)``).
+  The type of ``USHRT_MAX`` on AVR is now ``unsigned int`` instead of ``int``,
----------------
I'd be surprised if this actually has much of a chance to break people. I'd probably put this under a new AVR section under `Target Specific Changes`, WDYT?


================
Comment at: clang/lib/Headers/limits.h:55
 #define UCHAR_MAX (__SCHAR_MAX__*2  +1)
-#define USHRT_MAX (__SHRT_MAX__ *2  +1)
+#if __SHRT_MAX__ * 2U + 1U <= __INT_MAX__
+#define USHRT_MAX (__SHRT_MAX__ * 2 + 1)
----------------
Rather than do math here, WDYT about using the WIDTH macros? It seems like a more direct comparison:
```
#if __SHRT_WIDTH__ == __INT_WIDTH__
#endif
```
(This also makes me wonder if we have any targets where we need to do this with `UCHAR_MAX` as well. IIRC, we only support `CHAR_BIT` == 8 and so we're probably fine, but might as well double-check since we're fixing this kind of mistake.)


================
Comment at: clang/lib/Headers/limits.h:55
 #define UCHAR_MAX (__SCHAR_MAX__*2  +1)
-#define USHRT_MAX (__SHRT_MAX__ *2  +1)
+#define USHRT_MAX (__SHRT_MAX__ * 2U + 1U)
 #define UINT_MAX  (__INT_MAX__  *2U +1U)
----------------
mysterymath wrote:
> aaron.ballman wrote:
> > mysterymath wrote:
> > > aaron.ballman wrote:
> > > > It's worth double-checking that this still gives the correct type for the macro:
> > > > 
> > > > C2x 5.2.4.2.1p2: For all unsigned integer types for which <limits.h> or <stdint.h> define a macro with suffix _WIDTH holding its width N, there is a macro with suffix _MAX holding the maximal value 2N − 1 that is representable by the type and that has the same type as would an expression that is an object of the corresponding type converted according to the integer promotions. ...
> > > Ah, thanks; it hadn't occurred to me that the type of the expression would be specified in the standard. It could be either `unsigned int` or `int`, depending on the target.
> > > 
> > > The most straightforward approach I could think of to produce the right type is:
> > > 1) Perform the arithmetic in `unsigned int` to produce the right value
> > > 2) Cast to `unsigned short` to produce the right type
> > > 3) Directly perform integer promotion using unary plus
> > > 
> > > The use of unary plus is a bit odd here, but it seems like the most direct way to express the logic, and the overall expression seems less fragile than the `#if` alternative. I've added a comment to explain this as well.
> > Now the trouble is with the cast operation, because that runs afoul of 5.2.4.2.1p1: The values given below shall be replaced by constant expressions suitable for use in conditional expression inclusion preprocessing directives. ...
> > 
> > https://godbolt.org/z/K9cs66sdK
> > 
> > I'm almost wondering if the most direct solution is for `__SHRT_MAX__` to be generated with or without the `U` suffix depending on target.
> > 
> > We should probably use this as an opportunity to add more exhaustive testing for all the _MIN and _MAX macros to ensure the type properties hold. I was playing around with something like this: https://godbolt.org/z/o7KjY3asW
> > Now the trouble is with the cast operation, because that runs afoul of 5.2.4.2.1p1: The values given below shall be replaced by constant expressions suitable for use in conditional expression inclusion preprocessing directives. ...
> > 
> > https://godbolt.org/z/K9cs66sdK
> > 
> Sigh, it appears my standards-fu is not strong. I'm rather surprised to find that you can't cast to an integer type in `#if`; 6.10.1.6 claim they take a `constant-expression`, evaluated according to 6.6. But I can't find anything in 6.6 that seems to exclude cast operators. The definition of integer constant expressions even specifically calls out casts to integer types, although it doesn't specifically say in 6.10.1.6 that it takes an integer constant expression.
> Would you happen to know off-hand what my mistake is?
> 
> I think the `#if` version I had originally isn't subject to this; I'll switch back to that. 
> 
> > I'm almost wondering if the most direct solution is for `__SHRT_MAX__` to be generated with or without the `U` suffix depending on target.
> Seems like that wouldn't work for the more straightforward case of `#define SHRT_MAX __SHRT_MAX__`; the type of this must always be int, since SHRT_MAX is always representable in int, even on 16-bit targets.
>  
> > We should probably use this as an opportunity to add more exhaustive testing for all the _MIN and _MAX macros to ensure the type properties hold. I was playing around with something like this: https://godbolt.org/z/o7KjY3asW
> Agree; I think a slight modification to the type test macros I added to the limits.cpp test can accomodate this. Seems like it would also be a good idea to verify 5.2.4.2.1p1 directly by using the values in the preprocessor.
> Sigh, it appears my standards-fu is not strong. 

No worries, this is what code review is intended to catch! :-)

> I'm rather surprised to find that you can't cast to an integer type in #if; 6.10.1.6 claim they take a constant-expression, evaluated according to 6.6. But I can't find anything in 6.6 that seems to exclude cast operators. The definition of integer constant expressions even specifically calls out casts to integer types, although it doesn't specifically say in 6.10.1.6 that it takes an integer constant expression.
> Would you happen to know off-hand what my mistake is?

The preprocessor has no notion of types, just numbers, identifiers, and punctuation. Any identifier that isn't known to the preprocessor is replaced by `0`, which is what allows code like this to compile: https://godbolt.org/z/P3rzG8sxo

So your previous macro would expand to `(+(0 0)(0x7FFF * 2U + 1U))` and the preprocessor would get baffled.

> Seems like that wouldn't work for the more straightforward case of #define SHRT_MAX __SHRT_MAX__; the type of this must always be int, since SHRT_MAX is always representable in int, even on 16-bit targets.

Good point!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144218/new/

https://reviews.llvm.org/D144218



More information about the cfe-commits mailing list