[limits.h] USHRT_MAX fix for 16 bit architectures

[SebastianPerta]

Summary

On 16 bit architectures (where short is same size as int) USHRT_MAX overflows as it is currently defined.
warning: overflow in expression; result is -2 with type 'int' [-Winteger-overflow]
The attached patch fixes this issue.

[aaron.ballman]

Oooh, good catch! The changes, as far as they go, look good to me. But can you add some test coverage for this? Also, this should probably have a release note in clang/docs/ReleaseNotes.rst.

[erichkeane]

Please include the full context of the patch with -U99999. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[SebastianPerta]

>>But can you add some test coverage for this?
In clang\test\Headers\limits.cpp there is:
_Static_assert(USHRT_MAX == (unsigned short)~0ULL, "");
However as I said this needs a 16 bits target. Looking at the RUN commands at the top of the file I think I can solve this by adding a new RUN command with "-triple msp430" (since my RL78 port is not upstream yet) so it can catch this. Would this be OK?

Please include the full context of the patch with -U99999.

Sorry about this, I actually tried this but the patch showed a lot more than what I've changed, not sure what I did wrong. I also installed arcanist and I'm trying to get familiar with it.

[aaron.ballman]

In D129689#3651965, @SebastianPerta wrote:

>>But can you add some test coverage for this?
In clang\test\Headers\limits.cpp there is:
_Static_assert(USHRT_MAX == (unsigned short)~0ULL, "");
However as I said this needs a 16 bits target. Looking at the RUN commands at the top of the file I think I can solve this by adding a new RUN command with "-triple msp430" (since my RL78 port is not upstream yet) so it can catch this. Would this be OK?

That's exactly the solution I'd expect, sounds great!

Please include the full context of the patch with -U99999.

Sorry about this, I actually tried this but the patch showed a lot more than what I've changed, not sure what I did wrong. I also installed arcanist and I'm trying to get familiar with it.

No worries, it takes a while to get used to our systems. I've not use arcanist myself (I generate diff files and upload them manually into Phab), so I'm not much help for debugging what's going on for you. I use git diff -U9999 main > p.patch, FWIW.

[cor3ntin]

In D129689#3652010, @aaron.ballman wrote:

In D129689#3651965, @SebastianPerta wrote:

>>But can you add some test coverage for this?
In clang\test\Headers\limits.cpp there is:
_Static_assert(USHRT_MAX == (unsigned short)~0ULL, "");
However as I said this needs a 16 bits target. Looking at the RUN commands at the top of the file I think I can solve this by adding a new RUN command with "-triple msp430" (since my RL78 port is not upstream yet) so it can catch this. Would this be OK?

That's exactly the solution I'd expect, sounds great!

Please include the full context of the patch with -U99999.

Sorry about this, I actually tried this but the patch showed a lot more than what I've changed, not sure what I did wrong. I also installed arcanist and I'm trying to get familiar with it.

No worries, it takes a while to get used to our systems. I've not use arcanist myself (I generate diff files and upload them manually into Phab), so I'm not much help for debugging what's going on for you. I use git diff -U9999 main > p.patch, FWIW.

arc diff "HEAD^" --update D129689 on the branch you want to push should do what you want, assuming you only have 1 commit on that branch - I usually amend my commits for simplicity.

(in the future, --update XXXX is not necessary if you let arc create the patch initially)

[tahonermann]

Sorry about this, I actually tried this but the patch showed a lot more than what I've changed, not sure what I did wrong.

You didn't do anything wrong; the -U99999 option will effectively include the entire file for each changed file. That is actually the intent; it is what enables Phabricator to have access to the rest of the file so that reviewers can expand the displayed context to see unchanged code.

[aaron.ballman]

This seems to have fallen through the cracks -- @SebastianPerta are you planning to update the patch, or would you be okay if I commandeered it?

[SebastianPerta]

>>are you planning to update the patch
Yes, sorry about the delay. I will update the patch before the end of the week.

[cor3ntin]

@aaron.ballman I think we should close this change as it's superseeded by https://reviews.llvm.org/D144218

[SebastianPerta]

@cor3ntin can I close it?

[aaron.ballman]

In D129689#4194096, @SebastianPerta wrote:

@cor3ntin can I close it?

You can abandon this PR, it was handled by 0fecac18ffad476b5a4682770f6d8b1f0f176b40.

clang/lib/Headers/limits.h

 #define LONG_MIN  (-__LONG_MAX__ -1L)
 
 #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)
 #define ULONG_MAX (__LONG_MAX__ *2UL+1UL)