Navigation Menu

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

Change Word64 type from long long to unsigned long long #5458

Merged
merged 1 commit into from Sep 23, 2014

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Sep 21, 2014

The following change is required to solve undefined behavior detected by
UBSan.

405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:17:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:18:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:19:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:20:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:21:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:22:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:23:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:24:22
405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:25:22

~Word64(0) is -1 and left shifting a negative number is undefined
behavior by C++.

These produce 3645 runtime errors from UBSan on full matrix. That's ~15%
of all detected runtime errors.

Word32 is defined as unsigned type. Keep it consistent and define
Word64 as unsigned long long, thus resolved undefined behavior.

Looking at the code these 64-bit masks are not used outside this package
and they are only used with bitwise and operators. The change should not
have any side effects.

Test case to prove that it does not affect the masks and solves the issue:

$ cat -n t3.cpp
     1  #include <iostream>
     2
     3  typedef long long Word64;
     4  typedef unsigned long long UWord64;
     5
     6  int main(void) {
     7    Word64 m1, m2, m4, m5, m6, m8, m12, m16, m32;
     8    m1  = ~(~Word64(0) << 1);
     9    m2  = ~(~Word64(0) << 2);
    10    m4  = ~(~Word64(0) << 4);
    11    m5  = ~(~Word64(0) << 5);
    12    m6  = ~(~Word64(0) << 6);
    13    m8  = ~(~Word64(0) << 8);
    14    m12 = ~(~Word64(0) << 12);
    15    m16 = ~(~Word64(0) << 16);
    16    m32 = ~(~Word64(0) << 32);
    17    std::cout << std::hex << m1 << " " << m2 << " " << m4 << " " << m5 << " " << m6 << " " << m8 << " " << m12 << " " << m16 << " " << m32 << std::endl;
    18    UWord64 new1, new2, new4, new5, new6, new8, new12, new16, new32;
    19    new1  = ~(~UWord64(0) << 1);
    20    new2  = ~(~UWord64(0) << 2);
    21    new4  = ~(~UWord64(0) << 4);
    22    new5  = ~(~UWord64(0) << 5);
    23    new6  = ~(~UWord64(0) << 6);
    24    new8  = ~(~UWord64(0) << 8);
    25    new12 = ~(~UWord64(0) << 12);
    26    new16 = ~(~UWord64(0) << 16);
    27    new32 = ~(~UWord64(0) << 32);
    28    std::cout << std::hex << new1 << " " << new2 << " " << new4 << " " << new5 << " " << new6 << " " << new8 << " " << new12 << " " << new16 << " " << new32 << std::endl;
    29
    30    return 0;
    31  }
$ clang++ -g -fno-omit-frame-pointer -std=c++11 -fsanitize=undefined -std=c++11 t3.cpp
$ ./a.out
t3.cpp:8:22: runtime error: left shift of negative value -1
t3.cpp:9:22: runtime error: left shift of negative value -1
t3.cpp:10:22: runtime error: left shift of negative value -1
t3.cpp:11:22: runtime error: left shift of negative value -1
t3.cpp:12:22: runtime error: left shift of negative value -1
t3.cpp:13:22: runtime error: left shift of negative value -1
t3.cpp:14:22: runtime error: left shift of negative value -1
t3.cpp:15:22: runtime error: left shift of negative value -1
t3.cpp:16:22: runtime error: left shift of negative value -1
1 3 f 1f 3f ff fff ffff ffffffff
1 3 f 1f 3f ff fff ffff ffffffff

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

The following change is required to solve undefined behavior detected by
UBSan.

    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:17:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:18:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:19:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:20:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:21:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:22:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:23:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:24:22
    405 EventFilter/ESRawToDigi/src/ESUnpacker.cc:25:22

~Word64(0) is -1 and left shifting a negative number is undefined
behavior by C++.

These produce 3645 runtime errors from UBSan on full matrix. That's ~15%
of all detected runtime errors.

Word32 is defined as unsigned type. Keep it consistent and define
Word64 as unsigned long long, thus resolved undefined behavior.

Looking at the code these 64-bit masks are not used outside this package
and they are only used with bitwise and operators. The change should not
have any side effects.

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlt for CMSSW_7_2_X.

Change Word64 type from long long to unsigned long long

It involves the following packages:

EventFilter/ESRawToDigi

@cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Sep 23, 2014

+1

for #5458 20ac404
jenkins is OK
also checked in CMSSW_7_2_X_2014-09-22-1400 (test area sign433)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_2_X IBs unless changes (tests are also fine). @nclopezo can you please take care of it?

davidlange6 added a commit that referenced this pull request Sep 23, 2014
Change Word64 type from long long to unsigned long long
@davidlange6 davidlange6 merged commit 513c1a2 into cms-sw:CMSSW_7_2_X Sep 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants