Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to avoid undefined behavior in runtime intrinsics #31548

Merged
merged 1 commit into from May 20, 2019

Conversation

2 participants
@Keno
Copy link
Member

commented Mar 29, 2019

The analyzer complains

/home/keno/julia/src/runtime_intrinsics.c:886:38: note: The result of the left shift is undefined because the left operand is
      negative
checked_iintrinsic_fast(LLVMSub_sov, check_ssub_int, sub, checked_ssub_int,  )
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:510:25: note: expanded from macro 'checked_iintrinsic_fast'
checked_intrinsic_ctype(CHECK_OP, OP, name, 64, u##int##64_t) \
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:251:12: note: expanded from macro '\
checked_intrinsic_ctype'
    return CHECK_OP(a, b); \
           ^~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:885:49: note: expanded from macro 'check_ssub_int'
        (b >= 0) ? (a < sTYPEMIN(a) + b) : (a > sTYPEMAX(a) + b)
                                                ^~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:868:23: note: expanded from macro 'sTYPEMAX'
     ? ~((a - a + ~0) << (8 * sizeof(a) - 1)) \
         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
16 warnings generated.

And indeed shifting a negative number to the left is undefined in C.
Try an alternative expression for these that hopefully steers clear of any UB.

@Keno Keno requested a review from vtjnash Mar 29, 2019

@vtjnash

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

And indeed shifting a negative number to the left is undefined in C.

Not anymore! https://twitter.com/jfbastien/status/989242576598327296?lang=en

@vtjnash

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

Note that the analyzer appears to be wrong here regardless of whether that wording changes gets into C20 or merely just C++20, I only know K&R C and C99+ (e.g. I don't have a copy of ANSI or C89), but in those sources, it always had defined behavior in C (unlike C++, where it was UB):

http://www.coding-guidelines.com/C99/html/6.5.7.html [emp. mine]

1186 If E1 has a signed type and a negative value, the resulting value is implementation-defined.

@Keno

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2019

That's for right shift. The rule for left shift is:

1182 If E1 has a signed type and nonnegative value, and E1 × 2E2 is representable in the result type, then that is the resulting value;

1183 otherwise, the behavior is undefined.
@Keno

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

So what do we do about this. Any better suggestion for a version that is legal in C before next year?

@Keno

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Bump! I'd like to get this resolved so we can turn on the static analyzer. Alternatively, we can hide these expressions from the static analyzer, but I do think the static analyzer is correct about the undefinedness here.

@vtjnash

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Sure, seems fine to change. I'm not sure about your math though.

* u_typemin = 0
* u_typemax = ((typeof a)1 << runtime_nbits) - 1
* where (a - a) == (typeof(a)0
**/
#define sTYPEMIN(a) \
(8 * sizeof(a) == runtime_nbits \
? ((a - a + ~0) << (8 * sizeof(a) - 1)) \
: ((a - a + ~0) << (runtime_nbits - 1)))
? -(sTYPEMAX(a) - 1) \

This comment has been minimized.

Copy link
@vtjnash

vtjnash Apr 23, 2019

Member
Suggested change
? -(sTYPEMAX(a) - 1) \
(-sTYPEMAX(a) - 1)

This comment has been minimized.

Copy link
@Keno

Keno May 11, 2019

Author Member

did you mean to delete the ??

This comment has been minimized.

Copy link
@vtjnash

vtjnash May 11, 2019

Member

Yeah, it’s not conditional, it’s just the definition. (I think)

? ((a - a + ~0) << (8 * sizeof(a) - 1)) \
: ((a - a + ~0) << (runtime_nbits - 1)))
? -(sTYPEMAX(a) - 1) \
: ((a - a + 1) << (runtime_nbits - 1)))

This comment has been minimized.

Copy link
@vtjnash

vtjnash Apr 23, 2019

Member
Suggested change
: ((a - a + 1) << (runtime_nbits - 1)))
     : ~(((a - a + 1) << (runtime_nbits - 1))) - 1)
? ~((a - a + ~0) << (8 * sizeof(a) - 1)) \
: ~((a - a + ~0) << (runtime_nbits - 1)))
? ( ((((a - a + 1) << (8 * sizeof(a) - 2)) - 1 ) << 1) + 1 ) \
: ( ((a - a + 1) << (runtime_nbits - 1)) - 1))

This comment has been minimized.

Copy link
@vtjnash

vtjnash Apr 23, 2019

Member
Suggested change
: ( ((a - a + 1) << (runtime_nbits - 1)) - 1))
: ( ((a - a + 1) << (runtime_nbits - 1)) - 1))
#define sTYPEMAX(a) \
(8 * sizeof(a) == runtime_nbits \
? ~((a - a + ~0) << (8 * sizeof(a) - 1)) \
: ~((a - a + ~0) << (runtime_nbits - 1)))
? ( ((((a - a + 1) << (8 * sizeof(a) - 2)) - 1 ) << 1) + 1 ) \

This comment has been minimized.

Copy link
@vtjnash

vtjnash Apr 23, 2019

Member
Suggested change
? ( ((((a - a + 1) << (8 * sizeof(a) - 2)) - 1 ) << 1) + 1 ) \
? ( ((((a - a + 1) << (8 * sizeof(a) - 2)) - 1) << 1) + 1 ) \
@@ -853,20 +853,20 @@ un_fintrinsic_withtype(fpext,fpext)

// checked arithmetic
/**
* s_typemin = ((typeof a)~0 << (runtime_nbits - 1))
* s_typemax = ~((typeof a)1 << (runtime_nbits - 1))
* s_typemin = ((typeof a)1 << (runtime_nbits - 1))

This comment has been minimized.

Copy link
@vtjnash

vtjnash Apr 23, 2019

Member
Suggested change
* s_typemin = ((typeof a)1 << (runtime_nbits - 1))
* s_typemin = -s_typemax - 1
Try to avoid undefined behavior in runtime intrinsics
The analyzer complains
```
/home/keno/julia/src/runtime_intrinsics.c:886:38: note: The result of the left shift is undefined because the left operand is
      negative
checked_iintrinsic_fast(LLVMSub_sov, check_ssub_int, sub, checked_ssub_int,  )
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:510:25: note: expanded from macro 'checked_iintrinsic_fast'
checked_intrinsic_ctype(CHECK_OP, OP, name, 64, u##int##64_t) \
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:251:12: note: expanded from macro '\
checked_intrinsic_ctype'
    return CHECK_OP(a, b); \
           ^~~~~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:885:49: note: expanded from macro 'check_ssub_int'
        (b >= 0) ? (a < sTYPEMIN(a) + b) : (a > sTYPEMAX(a) + b)
                                                ^~~~~~~~~~~
/home/keno/julia/src/runtime_intrinsics.c:868:23: note: expanded from macro 'sTYPEMAX'
     ? ~((a - a + ~0) << (8 * sizeof(a) - 1)) \
         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
16 warnings generated.
```

And indeed shifting a negative number to the left is undefined in `C`.
Try an alternative expression for these that hopefully steers clear of any UB.

@Keno Keno force-pushed the kf/runtimintrub branch from e7ef666 to e4f75fd May 18, 2019

@Keno

This comment has been minimized.

Copy link
Member Author

commented May 18, 2019

Updated to address review

@Keno

This comment has been minimized.

Copy link
Member Author

commented May 19, 2019

@vtjnash good to go?

@vtjnash
Copy link
Member

left a comment

I think those are the right formulae.

@Keno Keno merged commit 047f9ee into master May 20, 2019

13 of 14 checks passed

buildbot/tester_win32 Run complete
Details
buildbot/package_freebsd64 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_macos64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/package_win64 Run complete
Details
buildbot/tester_freebsd64 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
buildbot/tester_macos64 Run complete
Details
buildbot/tester_win64 Run complete
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@KristofferC KristofferC deleted the kf/runtimintrub branch May 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.