Project

General

Profile

Bug #11844

Unsafe implementation of bitwise circular shift in cetlib/bit_manipulation.h

Added by Ben Morgan over 4 years ago. Updated about 4 years ago.

Status:
Closed
Priority:
Normal
Target version:
Start date:
03/01/2016
Due date:
% Done:

100%

Estimated time:
Spent time:
Duration:

Description

Unit tests of cet:circ_lshift revealed errors on clang for shifts of n=0 when compiled with optimizations higher than -O1. Though not traced directly in assembly, research shows that implementation of shift in cet::circ_lshift is non-portable as there is no protection against the undefined n=32=0 shifts. See, e.g.

http://blog.regehr.org/archives/1063

It is likely this has never been picked up as GCC recognizes the used implementation, even though it is is non-portable:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57157

Clang (and Intel's) rotation optimizations are likely different, resulting in the observed undefined behaviour at higher optimizations.

The attached patch updates the implementation of cet::circ_lshift to use Regehr/Cordes portable method to protect against null shifts:

http://blog.regehr.org/archives/1063
http://stackoverflow.com/questions/31387778/near-constant-time-rotate-that-does-not-violate-the-standards

This is currently un-optimized and there may be improvements should cet::circ_lshift be used in performance critical areas.

Associated revisions

Revision f7c62a4b (diff)
Added by Chris Green over 4 years ago

Implement change suggested in issue #11844

Prevent undefined behavior in circular left-shift.

History

#1 Updated by Marc Paterno over 4 years ago

  • Status changed from New to Accepted

Thanks for spotting this. We'll put in the suggested modifications (maybe with a change from from static const to constexpr).

#2 Updated by Kyle Knoepfel over 4 years ago

  • Assignee set to Christopher Green

#3 Updated by Christopher Green over 4 years ago

  • Status changed from Accepted to Resolved
  • Target version set to 2.01.00
  • % Done changed from 0 to 100

Implemented (with formatting changes and static const -> constexpr) with f7c62a4.

#4 Updated by Kyle Knoepfel about 4 years ago

  • Target version changed from 2.01.00 to 2.00.01

#5 Updated by Kyle Knoepfel about 4 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF