Issue 16521 - Wrong code generation with switch + static foreach
Summary: Wrong code generation with switch + static foreach
Status: RESOLVED INVALID
Alias: None
Product: D
Classification: Unclassified
Component: dmd (show other issues)
Version: D2
Hardware: x86_64 Linux
: P1 major
Assignee: No Owner
URL:
Keywords:
: 17218 (view as issue list)
Depends on:
Blocks:
 
Reported: 2016-09-21 19:54 UTC by Mathias Lang
Modified: 2021-03-12 16:31 UTC (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description Mathias Lang 2016-09-21 19:54:33 UTC
The following code:

```
void main ()
{
    sformat(0, uint.max);
}

void sformat (Args...) (int index, Args args)
{
JT: switch (index)
    {
        foreach (idx, unused; args)
        {
        case idx:
            assert(unused == args[idx], "Borken compiler");
            break JT;
        }

    default:
        assert(0);
    }
}
```

Triggers the assert.
Obviously `unused` and `args[idx]` should always be the same.
Here's the generated ASM:

```
000000000042237c <D main>:
  42237c:       55                      push   %rbp
  42237d:       48 8b ec                mov    %rsp,%rbp
  422380:       31 f6                   xor    %esi,%esi
  422382:       bf ff ff ff ff          mov    $0xffffffff,%edi
  422387:       e8 08 00 00 00          callq  422394 <test.sformat!(uint).sformat(int, uint)>
  42238c:       31 c0                   xor    %eax,%eax
  42238e:       5d                      pop    %rbp
  42238f:       c3                      retq   
  422390:       0f 1f 40 00             nopl   0x0(%rax)

0000000000422394 <test.sformat!(uint).sformat(int, uint)>:
  422394:       55                      push   %rbp
  422395:       48 8b ec                mov    %rsp,%rbp
  422398:       48 83 ec 20             sub    $0x20,%rsp
  42239c:       89 7d f8                mov    %edi,-0x8(%rbp)
  42239f:       85 f6                   test   %esi,%esi
  4223a1:       74 02                   je     4223a5 <test.sformat!(uint).sformat(int, uint)+0x11>
  4223a3:       eb 37                   jmp    4223dc <test.sformat!(uint).sformat(int, uint)+0x48>
  4223a5:       8b 45 f0                mov    -0x10(%rbp),%eax ; <== Access to unitialized variable `unused`
  4223a8:       3b 45 f8                cmp    -0x8(%rbp),%eax
  4223ab:       74 2d                   je     4223da <test.sformat!(uint).sformat(int, uint)+0x46>
  4223ad:       41 b8 0d 00 00 00       mov    $0xd,%r8d
  4223b3:       b9 00 54 44 00          mov    $0x445400,%ecx
  4223b8:       b8 06 00 00 00          mov    $0x6,%eax
  4223bd:       48 89 c2                mov    %rax,%rdx
  4223c0:       48 89 55 e8             mov    %rdx,-0x18(%rbp)
  4223c4:       ba 07 54 44 00          mov    $0x445407,%edx
  4223c9:       bf 0f 00 00 00          mov    $0xf,%edi
  4223ce:       48 89 d6                mov    %rdx,%rsi
  4223d1:       48 8b 55 e8             mov    -0x18(%rbp),%rdx
  4223d5:       e8 ce 00 00 00          callq  4224a8 <_d_assert_msg>
  4223da:       eb 0a                   jmp    4223e6 <test.sformat!(uint).sformat(int, uint)+0x52>
  4223dc:       bf 12 00 00 00          mov    $0x12,%edi
  4223e1:       e8 2e 00 00 00          callq  422414 <test.__assert(int)>
  4223e6:       c9                      leaveq 
  4223e7:       c3                      retq   
  4223e8:       0f 1f 40 00             nopl   0x0(%rax)
```

Tested with v2.070.0 and v2.071.1
The latest master (cce7909) hints at the bug:
test.d(8): Deprecation: 'switch' skips declaration of variable test.sformat!uint.sformat.unused at test.d(10)
Comment 1 Mathias Lang 2016-09-21 20:59:15 UTC
A bit of debugging proved that the following code is generated:

```
[{
enum ulong idx = 0;
uint unused = _param_1;
case idx:
{
assert(unused == args[idx], "Borken compiler");
break JT;
}
}
]
```

Note: Using `ref` results in the program segfaulting because it will then dereference an uninitialized `ref`.
Comment 2 Nemanja Boric 2017-01-30 13:37:30 UTC
I hit the same bug today:

```
import std.traits;

struct S
{
    int a, b, c, d, e, f;
}

void main()
{
    S s;

    foreach (name; ["a", "b", "c"])
    {
        switch (name)
        {
            foreach (i, ref field; s.tupleof)
            {
                case __traits(identifier, S.tupleof[i]):
                    field = 0; // s.tupleof[i] = 0; works
                    break;
            }
            default:
                break;
        }
    }
}
```
Comment 3 Steven Schveighoffer 2017-03-09 19:03:44 UTC
I see the same deprecation warning in my code, but I'm not using the actual tuple value, just the index.

Static foreach would be handy here...
Comment 4 Stefan Koch 2017-03-09 19:25:42 UTC
There is no such thing as static foreach.
You are using tuple foreach which will force an unrolled loop.

The provided code should error!
Comment 5 Nemanja Boric 2017-03-09 20:00:24 UTC
I don't agree that this code should error. This is a well known and common D idiom (simple GH search shows examples in Phobos, such: https://github.com/dlang/phobos/blob/master/std/algorithm/sorting.d#L1101), up to the point that there are also merged switch/foreach loops constructs where the label is applied to switch, so the `break` inside foreach would be considered switch break.
Comment 6 Stefan Koch 2017-03-09 20:06:14 UTC
Just because something is used in phobos does not it is correct.
this code is highly dubious because it goats people into believing that tuple foreach actually works.

The right way to do this is to build a string for the switch and mix it in.
Comment 7 Nemanja Boric 2017-03-09 20:21:58 UTC
I'm not sure who ever got convinced by this code that `static foreach` works. This is just very helpful and clear approach, and it should grant `static foreach` becoming a real thing, because it is useful.

While that is still not implemented, this code should work, as I see this as a bug/regression (https://github.com/rejectedsoftware/vibe.d/blob/6f37e694cc77063769bc4c9a42160627103e8354/web/vibe/web/rest.d#L1367 - yet another example, plus dozens on forums, etc).
Comment 8 Steven Schveighoffer 2017-03-09 20:58:00 UTC
guys, the static foreach comment was a *wish* for this, not a recommendation to use some already existing thing.

It would be nice to do:

static foreach(idx; 0 .. args.length)

Instead of the goofy "unused" symbol thing. In my code, I'm using Args, not args, so I have no idea why a variable for each tuple item should even need to be created. It might even be a type!

Note that the "deprecation" is what I'm concerned about, as I have only this one way to make my switch statement out of a tuple (sorry Stefan, but string mixin is a *vastly inferior* option compared to this mechanism), and some future version of dmd I worry is going to flag this as an error, even though I never use the variable I didn't want to declare but was forced to.
Comment 9 Alex Goltman 2017-04-07 11:14:19 UTC
looks like a duplicate of my issue https://issues.dlang.org/show_bug.cgi?id=17218
Comment 10 Steven Schveighoffer 2017-04-23 13:38:34 UTC
*** Issue 17218 has been marked as a duplicate of this issue. ***
Comment 11 Seb 2018-02-09 19:38:44 UTC
FWIW this works with static foreach


---
void main ()
{
    sformat(0, uint.max);
}

void sformat (Args...) (int index, Args args)
{
JT: switch (index)
    {
        static foreach (idx, unused; args)
        {
        case idx:
            assert(unused == args[idx], "Borken compiler");
            break JT;
        }

    default:
        assert(0);
    }
}
---

https://run.dlang.io/is/eJvNFB
Comment 12 Imperatorn 2021-03-10 12:22:17 UTC
Any update on this?
Comment 13 Steven Schveighoffer 2021-03-12 16:31:20 UTC
(In reply to Imperatorn from comment #12)
> Any update on this?

What do you mean? It's resolved as invalid. The code no longer compiles (for a while now).