pass dims to range error runtime function for better exception messages

[adamdruppe]

This is something that has bugged me for many, many years: the compiler knows what the out-of-bounds index was, but doesn't bother to tell us, leading to a bunch of wasted time trying to figure out what random input caused the error. At least knowing the index and length is a solid starting point in reproducing it in a debugger.

This PR is the dmd side of passing that information over to the runtime. I know it could still use a bit of polish (especially for associative arrays, I think something else entirely needs to be done for them, though passing all zeroes could be detected on the runtime side and change the message too - at least it wouldn't be any worse than we have now).

But, before spending the time on that: are you guy actually willing to accept this?

[CyberShadow]

I am no compiler expert but would el_copytree cause the expressions to be re-evaluated (incl. their side effects)? E.g. int[1] a; int i = 5; a[++i] = 1;

[adamdruppe]

Honestly, I have no idea. I know they copied other things in the function with that, and if I don't copy it, the compiler dies with an internal error when I try to use it... that's one of the reasons I opened the PR now, to get someone who knows what they're doing to weigh in.

[WalterBright]

@CyberShadow is correct.

[adamdruppe]

@WalterBright What's the right way to do it?

[CyberShadow]

I think you have to save the value to a temporary first.

[yebblies]

There's a copytotemp function somewhere around.

[WalterBright]

Yes, but I suggest waiting on fixing that, as I am opposed to this PR on performance grounds.

[WalterBright]

While I agree with the sentiment, this produces substantial runtime bloat, even if no range errors occur. Take a look at the generated code, and multiply it by all the places where arrays are accessed.

[adamdruppe]

Building druntime debug on my computer with dmd before and after this patch resulted in a difference of about 8 KB in a 16 MB file, or a difference of ~0.05%. (obviously, the release build is unchanged). If it is a hot loop where you're counting your cache lines, you'll probably use the .ptr trick or something to selectively disable bounds checking anyway, and outside such places, having the info is worth the cost to me.

I haven't tried other programs, but you should - I implore you not to let your gut take the place of actual measurements. If you can prove to me that there actually is substantial runtime bloat, I'll close it (and keep the patch for my local dmd fork), but I insist on seeing reproducible numbers.

[CyberShadow]

I could do some tests once you fix the side effects (otherwise it wouldn't be accurate of the end result).

I'm guessing range checking could theoretically be lowered to a Druntime templated function call... but that would probably really slow down compilation and require that the function is inlined.

[adamdruppe]

On Thu, May 18, 2017 at 12:35:19PM -0700, Vladimir Panteleev wrote: I could do some tests once you fix the side effects (otherwise it wouldn't be accurate of the end result).

Yeah, I'll look at that later today... though I expect that will make the generated code a bit smaller anyway, since the bounds need to be loaded up for the comparison all we're really costing is a `push EAX; push EBX;` or whatever to pass the numbers to the function after the `dim < length` check...

[WalterBright]

I implore you not to let your gut take the place of actual measurements.

Adding any instructions to an inner loop is expensive - and that's where one finds array accesses.

[WalterBright]

all we're really costing

No. The array index and array length have to be 'live' after the check, this has consequences for register allocation. It's worse if the index and length have side effects and must be put into temporaries. Those cost 2 registers even if the bounds check passes.

[WalterBright]

Generally, I don't want to make bounds checking expensive, as that will encourage people to turn it off (and those ignorant of compiler switches will declare D to be "worse code than C++"). Yes, I've seen the latter happen before.

[wilzbach]

Thank you @adamdruppe for pushing this. Just a dumb question: this does only affect the debug builds anyways, right? And isn't the entire point of debug builds to be slower, but easier to debug (hence the name)?

[adamdruppe]

On Fri, May 19, 2017 at 02:12:10AM -0700, Sebastian Wilzbach wrote: Just a dumb question: this does only affect the debug builds anyways, right?

Bounds checks are on unless you explicitly turn them off, which IMO you should NEVER do (with one rare exception, if you are doing runtime-less bare metal code in an extremely constrained environment). I consider the `-release` and `-boundscheck` switches to dmd to be EXTREMELY harmful and it pains me any time I see someone recommending them for a slight edge in a benchmark... they are D's #1 memory safety feature (with #2 btw being the garbage collector!) and production/release builds are where they are MOST important - that's where they are actually protecting you against actual data! So this change affects all builds except those horribly broken, memory-unsafe disasters waiting to happen. But, you can selectively disable bounds checks in individual expressions when you are very careful using the `.ptr` property: http://arsdnet.net/this-week-in-d/dec-06.html So when you are in an exceedingly tight loop and want max speed, you do it that way without throwing out the huge benefits the bounds checks give in the rest of the program. Of course, you also need to make this @trusted if you want to interact with @safe, so that's another reason to make sure you only do it where you actually need it and can be very careful in properly checking it. You should only be doing that when you have verified that 1) there's actually a speed problem and 2) you aren't overstepping your bounds.

[wilzbach]

Bounds checks are on unless you explicitly turn them off, which IMO you should NEVER do

My question was rather in the way: is there a chance we can pass this additional information on only for debug builds? AFAICT this would be a nice compromise and make everyone happy?

[dlang-bot]

Thanks for your pull request and interest in making D better, @adamdruppe! 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 + dmd#6805"

[wilzbach]

My question was rather in the way: is there a chance we can pass this additional information on only for debug builds? AFAICT this would be a nice compromise and make everyone happy?

Ping. AFAICT debugs builds can be slow, but should provide excellent debug information to a developer (exactly what this PR does). In release mode, no performance penalty is paid.

(I also rebased the PR)

[thewilsonator]

@WalterBright can you please reevaluate this PR in light of the merge of #7386 and #7392? @adamdruppe I take it that #5637 is redundant if this goes in?

[adamdruppe]

Yes, yes, yes, I actually considered making that change and reintroducing this a couple months ago, but then the day job got in the way again. Walter would probably be able to do it much faster and better than I would anyway.

I don't remember opening this twice though, lol. Been a while.

[adamdruppe]

I rebased and added ddoc to it too, hopefully it will pass the tests now and we can experiment a little.

BTW though remember, this just passes new arguments to the function. druntime will also need to be able to read those arguments and sink them to the user too. I don't recall if I did the separate druntime PR before or not.

[adamdruppe]

I guess the worry about side effect copying is still here, but I'm gonna wait till someone else comments to put any more time on it.

[CyberShadow]

That sounds like something that ought to be fixed before we can derive any meaningful results from tests, as duplicating the expressions could generate much more code than if they were saved to a temporary first.

[adamdruppe]

BTW I am a bit surprised that it is passing all the tests if it is actually duplicating code. I guess i should disassemble it with my own test case.

[thewilsonator]

Thanks for rebasing. There are no open PRs by you in druntime (if you open one make sure to use the same brach name as this will cause it to use DMD + this PR for testing).

If you could post an example of D + generated ASM of a loop with array access (with and without this PR) that would be great, assuming it doesn't add instructions to the loop I'm fine with merging this once we figure out whats the deal with copying stuff, (ideally any given function makes only a single call to the arrayBoundsError function, but I'll settle for not generating complete garbage) and the druntime PR is open and has tests.

[adamdruppe]

Well, Walter hasn't made the change to move bounds checking error code to outside the function yet, so it would still be inside the loop at this point, which I see in the asm.

But that's not surprising to me (yet, I assume Walter will get around to that later too). However, the side effect thing doesn't seem to be here:

void main() {
        int[] a;
        int c = 0;
        int b = a[c++];
}

Disassembly of section .text._Dmain:

00000000 <_Dmain>:
   0:   55                      push   ebp
   1:   8b ec                   mov    ebp,esp
   3:   83 ec 14                sub    esp,0x14
   6:   89 5d ec                mov    DWORD PTR [ebp-0x14],ebx
   9:   89 75 f0                mov    DWORD PTR [ebp-0x10],esi
   c:   31 c0                   xor    eax,eax
   e:   99                      cdq
   f:   89 45 f4                mov    DWORD PTR [ebp-0xc],eax
  12:   89 55 f8                mov    DWORD PTR [ebp-0x8],edx
  15:   89 45 fc                mov    DWORD PTR [ebp-0x4],eax
  18:   8b 4d fc                mov    ecx,DWORD PTR [ebp-0x4]
  1b:   ff 45 fc                inc    DWORD PTR [ebp-0x4]
  1e:   3b 4d f4                cmp    ecx,DWORD PTR [ebp-0xc]
  21:   72 12                   jb     35 <_Dmain+0x35>
  23:   ff 75 f4                push   DWORD PTR [ebp-0xc]
  26:   51                      push   ecx
  27:   50                      push   eax
  28:   6a 04                   push   0x4
  2a:   bb 00 00 00 00          mov    ebx,0x0
  2f:   53                      push   ebx
  30:   e8 fc ff ff ff          call   31 <_Dmain+0x31>
  35:   8b 55 f8                mov    edx,DWORD PTR [ebp-0x8]
  38:   8b 5d f4                mov    ebx,DWORD PTR [ebp-0xc]
  3b:   8b 34 8a                mov    esi,DWORD PTR [edx+ecx*4]
  3e:   8b 5d ec                mov    ebx,DWORD PTR [ebp-0x14]
  41:   8b 75 f0                mov    esi,DWORD PTR [ebp-0x10]
  44:   c9                      leave
  45:   c3                      ret

You can see the inc in there for the ++ before the range block, but inside it just says push ecx; - the dim - without a duplicated inc.

[thewilsonator]

Looks like el_same might do what you want.

[thewilsonator]

Looks like we need to copy what is done for throw, although I'm not sure how all the block stuff needs to be done. May need to convert the (c || arrayBoundsError) into a proper if/else to do that.

[wilzbach]

if you open one make sure to use the same brach name as this will cause it to use DMD + this PR for testing

Unfortunately that's only true for branches created at upstream at the moment.

[adamdruppe]

well, i changed it to use el_same but the block change thing idk about yet.

[thewilsonator]

Hmm, CI is red, maybe use el_copytotmp instead?

[thewilsonator]

Unfortunately that's only true for branches created at upstream at the moment.

Hmm, oh well, that will be more likely to get Walter's attention.

[adamdruppe]

lol the fails are build druntime "Error: variable _TMP1229 used before set" etc. So it is making a temporary and complaining about its own.

But with my original code all the tests passed so I'm not convinced it was wrong.

[adamdruppe]

So, taking a closer look at the surrounding code, the reason my old code is working is because there are already calls to el_same or el_copytotemp earlier in the functions. By the time we get to the lines I am modifying, the potential side effects are already in the past.

So that explains why the tests are passing with copytree - the code is correct!

[thewilsonator]

Good, so the next course of action is to closet this PR and open ones on upstream branches so that the druntime can be tested in sync with this, I'll do it later today.

[thewilsonator]

#9015