Add length, lower and upper bounds to RangeError/_d_arraybounds #2377

thewilsonator · 2018-11-26T13:10:37Z

No description provided.

dlang-bot · 2018-11-26T13:10:38Z

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2377"

thewilsonator · 2018-11-26T13:24:08Z

Ah, need to do phobos as well.

src/core/exception.d

thewilsonator · 2018-11-27T05:35:47Z

Hmm, test is being compiled with -release which is suppressing the call to _d_array_bounds.
e: Darn, looks like the only way around this is to change the makefiles, urgh.

jmdavis · 2018-11-27T05:50:42Z

Hmm, test is being compiled with -release which is suppressing the call to _d_array_bounds.
e: Darn, looks like the only way around this is to change the makefiles, urgh.

Mark the unittest as @safe.

thewilsonator · 2018-11-27T05:51:45Z

Thanks!

thewilsonator · 2018-11-27T05:55:05Z

Hmm, nope: Error: can only catch class objects derived from Exception in @safe code, not core.exception.RangeError

thewilsonator · 2018-11-27T05:57:45Z

Ahh, @safe lambdas work.

jmdavis · 2018-11-27T06:05:36Z

Hmm, nope: Error: can only catch class objects derived from Exception in @safe code, not core.exception.RangeError

That's nice. I wasn't expecting that to be the case, but it really should be.

Ahh, @safe lambdas work.

Yeah, if you can't make the whole thing @safe, then making the pieces you need @safe via lambdas should get around it. Either way, since -release should never be stripping out array bounds checking in @safe code, as long as the portions of the tests indexing arrays out-of-bounds are @safe, the checks should still be in place. If not, then it's a compiler bug.

thewilsonator · 2018-11-27T06:08:14Z

This doubles as a nice test for that then.

src/core/exception.d

thewilsonator · 2018-11-27T11:04:55Z

(I've altered the testes to figure out what indices are passed to _d_arraybounds.)

thewilsonator · 2018-11-27T11:57:32Z

core.exception.RangeError@src/core/exception.d(137): Range violation: indicies [6250512 .. 6250512] exceeds array length 6250512
Yeah that looks like garbage arguments.

src/core/exception.d

adamdruppe · 2018-12-07T23:44:13Z

I think you were over-zealous with the copy/paste of your print code rather than the arguments being garbage.

That length does seem a bit odd, but it being repeated is just it printing the same variable three times.

thewilsonator · 2019-01-07T08:40:41Z

CircleCI reports its triple as x86_64-pc-linux-gnu.

ibuclaw · 2019-01-07T08:52:00Z

src/core/exception.d

    }

-    void _d_arraybounds(string file, uint line)
+    void _d_arraybounds(string file, uint line, size_t lower, size_t upper, size_t length)


Rather than changing the signatures of the public extern C interface, new functions should be introduced.

Agreed. Then we don't have to risk merging this + the DMD change at the same time - this could be merged first, then the DMD patch can be merged if it passes CI.

For reference the DMD patch is already in, hence why I want to get this in. Also as noted by Jacob, we don't guarantee binary compatibility across releases.

See _aaGet, _aaGetX, _aaGetY for examples of doing this in the past. Infact pick any function that ends in X (_d_arrayliteralTX)

Even _d_arrayboundsp was a new name given to replace _d_arrayboundsm. :-)

But what's the point of doing so and creating more legacy cruft if we don't we don't guarantee binary compatibility across releases anyhow?

Call parameter stack layout: adding args to the end doesn't affect previous ones. I.e. passing too many will ignore the later ones, whereas passing too few will lead to garbage/corruption.

Great, so that means it works on x86, x86_64, and ... ? I don't think this calling convention is universal, is it?

x86 is the only target that pushes arguments in reverse. All others are left to right.

I have an ARM and Aarch64 server that I can run a test on. Regressions on those targets could warrant a revert... Did the druntime changes end up in a release?

Assuming that it's a dmd backend change only though, it's not so critical. If array bounds checks were being generated by the frontend, then it's another matter.

It is a backend thing.

jacob-carlborg · 2019-01-07T08:53:59Z

CircleCI reports its triple as x86_64-pc-linux-gnu.

Right, not sure what I was thinking.

thewilsonator · 2019-01-07T09:02:51Z

@ibuclaw signature of public functions is now compatible with default arguments. The functions you commented in are compiler generated.

ibuclaw · 2019-01-07T09:36:45Z

They will still silently break with applications linked to previous libraries. Just questioning whether this warrants a backwards incompatible ABI version bump.

jacob-carlborg · 2019-01-07T10:21:32Z

We don't guarantee any ABI compatibility as far as I know.

thewilsonator · 2019-01-08T02:10:43Z

Hey, it passes on CircleCI! Yay! I've no idea why that was failing before.

src/core/exception.d

changelog/range-error.dd

src/core/exception.d

CyberShadow · 2019-01-10T01:20:25Z

src/core/exception.d

+            }
+            catch (Throwable)
+            {
+                // ignore more errors


This code was copied from https://github.com/CyberShadow/druntime/blob/c6b6803646c24c8e748ff9961595c99cd99089cb/src/object.d#L2631-L2660 .

src/core/exception.d

CyberShadow · 2019-01-10T01:23:08Z

src/core/exception.d

    }

-    void _d_arraybounds(string file, uint line)
+    void _d_arraybounds(string file, uint line, size_t lower, size_t upper, size_t length)


Agreed. Then we don't have to risk merging this + the DMD change at the same time - this could be merged first, then the DMD patch can be merged if it passes CI.

CyberShadow · 2019-01-12T01:22:27Z

BTW, how did the DMD PR get merged without this one? Wouldn't the change in function signatures break the DMD/Druntime ABI?

thewilsonator · 2019-01-12T01:24:37Z

Because is passed the CI. Arguments added to the end of the parameter list don't collide with ones earlier in the list.

CyberShadow

Looking at this again - the if (upper) and if (lower) checks do not look good and generate inconsistent and potentially misleading messages.

For example:

auto a = (new int[0])[1]; // Range violation: index [1] exceeds array length 0
auto b = (new int[0])[0]; // Range violation

auto c = (new int[0])[1..1]; // Range violation: indicies [1 .. 1] exceeds array length 0
auto d = (new int[0])[0..1]; // Range violation: index [1] exceeds array length 0
auto e = (new int[0])[-1..0]; // Range violation

The d one in particular is outright misleading, we are not accessing index 1, only slicing up to it.

I think we should avoid misleading users, so that should probably need to be fixed before this is merged. What do you think?

CyberShadow · 2019-01-12T01:34:16Z

Suggested fix:

Introduce new C functions as suggested by @ibuclaw
Store separately whether we have zero (old API), one (index) or two (slice) indices
Format accordingly to number of indices we have

thewilsonator · 2019-01-12T11:32:42Z

a,b,c and e are edge cases, I will fix those soon.

d while potentially misleading, cross-referencing the line number will show a slice.

CyberShadow · 2019-01-12T11:42:52Z

a,b,c and e are edge cases, I will fix those soon.

Not sure what you mean by fix - a and c look correct, they are there for comparison.

I don't think there's a reasonable way to fix this in general without additional state / transmitted information. There are no free bits to hold information on whether the indices are actually indices, and the two indices don't necessarily have a relation to each other.

d while potentially misleading, cross-referencing the line number will show a slice.

That doesn't sound great. It's the worst case, and it might not be the only indexing operation on the line.

thewilsonator · 2019-01-12T11:46:02Z

Not sure what you mean by fix - a and c look correct, they are there for comparison.

My bad about a, the rest were wrong and I assumed that was too. c, unless you mispasted, is missing the the length of the array at the end.

CyberShadow · 2019-01-12T11:49:06Z

Yes, that was a mispaste, sorry.

dkorpel · 2021-03-31T08:38:54Z

I would love to get this in. Are you still going to work on this @thewilsonator?

thewilsonator · 2021-03-31T09:58:53Z

not with any sense of urgency.

12345swordy · 2021-03-31T15:59:41Z

How much left is needed for this to be done?

Geod24 · 2021-06-02T03:42:57Z

Closing for lack of activity (and to clean up upstream branches). The enhancement would be amazing to have, though.

thewilsonator requested a review from CyberShadow as a code owner November 26, 2018 13:10

thewilsonator requested review from jmdavis, MartinNowak and WalterBright as code owners November 26, 2018 13:10

jacob-carlborg reviewed Nov 26, 2018

View reviewed changes