Fix undefined behavior of BitsSet.popFront() upon aggressive optimization

[JohanEngelen]

A right-shift of a number of bits >= type's bits is invalid.
In this case, BitsSet was doing a >>>=32 on an int, leading to undefined behavior with aggressive optimizations in LDC. The solution in this PR is stopping further processing when the range has become empty by the popFront() call.

The bug is triggered by the assert(bitsSet(1).equal([0])); unittest already present, with LDC+aggressive inlining and optimization.

[dnadlinger]

Apparently this breaks on DMD's 32 bit targets. Seems valid at first glance, though, and it seems like there should be no way this causes a segfault?!

[JohanEngelen]

Weird errors. :(

[schveiguy]

Don't you need to increment _index by 1 at least?

[JohanEngelen]

Why? I thought when popFront leaves the range empty, front() can return garbage?

[schveiguy]

I'm just looking at it from a perspective of having identical behavior as before.

[schveiguy]

You still have a call to countTrailingZeros and bit shifting in the constructor, which could potentially do the same thing as popFront was doing.

[JohanEngelen]

identical behavior as before.

That would be index += 1 + sizeof(T)*8. I don't see why it is needed though.

[schveiguy]

Yeah, you are right. _index doesn't need to be incremented at that point.

[JohanEngelen]

You still have a call to countTrailingZeros and bit shifting in the constructor

Nice!! thanks.

[JohanEngelen]

Now that value cannot be 0 for the countTrailingZeros call, the call can be replaced with core.bitop.bsf and countTrailingZeros can be removed completely. Shall I do so?

[JohanEngelen]

@schveiguy updated!

[schveiguy]

Well, LGTM. Let's hope it passes the 32-bit checkers.

[dnadlinger]

Now that you are using bsf, this comment seems rather out-of-place (bsf would have been undefined anyway). I'd say the above one is enough. And I liked the early-return version better.

[dnadlinger]

@WalterBright @yebblies @MartinNowak: Any ideas about the 32 bit failures? Unless we are all missing something, this seems to be a misoptimization, ironically triggered by a fix to illegal code.

[codecov-io]

Current coverage is 88.67% (diff: 100%)

Merging #4509 into master will decrease coverage by <.01%

@@             master      #4509   diff @@
==========================================
  Files           121        121          
  Lines         73827      73810    -17   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          65471      65454    -17   
  Misses         8356       8356          
  Partials          0          0          

Powered by Codecov. Last update f75eeb5...4095762

[JohanEngelen]

(rebased)

[WalterWaldron]

@WalterBright @yebblies @MartinNowak: Any ideas about the 32 bit failures? Unless we are all missing something, this seems to be a misoptimization, ironically triggered by a fix to illegal code.

Looks like a DMD code gen bug, I've reduced & filed it here: https://issues.dlang.org/show_bug.cgi?id=17034

[WalterWaldron]

The fix was merged and seems to have worked.
The changes look good to me.

Time to move this forward?

[wilzbach]

The fix was merged and seems to have worked.

Thanks a lot @WalterWaldron!

The changes look good to me.

Same here -> +1

[wilzbach]

Seems valid at first glance,
...
Well, LGTM. Let's hope it passes the 32-bit checkers.
..
Time to move this forward?

As the fix was merged, it passes all CI checks, the change is fairly simply and nobody objected in the last seven days - I will go ahead and merge it, s.t. it will be part of the upcoming 2.073 release ;-)