Bug 18232 – Union methods fail to initialize local variables to .init

Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P1
Component
dmd
Product
D
Version
D2
Platform
x86_64
OS
Linux
Creation time
2018-01-12T11:08:42Z
Last change time
2018-01-15T06:25:45Z
Keywords
pull, wrong-code
Assigned to
No Owner
Creator
Stefan

Comments

Comment #0 by kdevel — 2018-01-12T11:08:42Z
crash.d --- import std.stdio; union U { float f; int i; string toString () { string s; return s; } } void main () { U u; writeln (u); } --- $ dmd crash $ dmd std.exception.ErrnoException@/.../dmd2/linux/bin64/../../src/phobos/std/stdio.d(2776): (Bad address) ---------------- ??:? @safe int std.exception.errnoEnforce!(int, "/.../dmd2/linux/bin64/../../src/phobos/std/stdio.d", 2776uL).errnoEnforce(int, lazy immutable(char)[]) [0x43f20a] ??:? @safe void std.stdio.File.LockingTextWriter.put!(immutable(char)[]).put(immutable(char)[]) [0x4422a7] ??:? @safe void std.range.primitives.doPut!(std.stdio.File.LockingTextWriter, immutable(char)[]).doPut(ref std.stdio.File.LockingTextWriter, ref immutable(char)[]) [0x44224b] ??:? @safe void std.range.primitives.put!(std.stdio.File.LockingTextWriter, immutable(char)[]).put(ref std.stdio.File.LockingTextWriter, immutable(char)[]) [0x44222b] ??:? void std.format.formatObject!(std.stdio.File.LockingTextWriter, crash.U, char).formatObject(ref std.stdio.File.LockingTextWriter, ref crash.U, ref const(std.format.FormatSpec!(char).FormatSpec)) [0x44220a] ??:? void std.format.formatValue!(std.stdio.File.LockingTextWriter, crash.U, char).formatValue(ref std.stdio.File.LockingTextWriter, ref crash.U, ref const(std.format.FormatSpec!(char).FormatSpec)) [0x44219d] ??:? uint std.format.formattedWrite!(std.stdio.File.LockingTextWriter, char, crash.U).formattedWrite(ref std.stdio.File.LockingTextWriter, const(char[]), crash.U) [0x43e703] ??:? void std.stdio.File.write!(crash.U, char).write(crash.U, char) [0x43e3f5] ??:? void std.stdio.writeln!(crash.U).writeln(crash.U) [0x43e389] ??:? _Dmain [0x43e344] http://forum.dlang.org/thread/[email protected]
Comment #1 by hsteoh — 2018-01-12T18:18:22Z
The crash is caused by wrong codegen for the toString() method. Here's a comparison of a toString() method for a struct vs. for a union: Struct code: -------- struct Jack { float f; int i; string toString() { string s; return s; } } -------- Generated assembly for Jack.toString: -------- 447dc: 55 push %rbp 447dd: 48 8b ec mov %rsp,%rbp 447e0: 31 c0 xor %eax,%eax 447e2: 31 d2 xor %edx,%edx 447e4: 5d pop %rbp 447e5: c3 retq -------- As can be easily seen, this basically initializes the string s to have .ptr=null, .length=0, returned by value in %eax and %edx. This is the correct behaviour. Union code (basically the same as the struct code, just change 'struct' to 'union'): -------- union Jack { float f; int i; string toString() { string s; return s; } } -------- Generated assembly for Jack.toString: -------- 4471c: 55 push %rbp 4471d: 48 8b ec mov %rsp,%rbp 44720: 48 83 ec 10 sub $0x10,%rsp 44724: 48 8b 55 f8 mov -0x8(%rbp),%rdx 44728: 48 8b 45 f0 mov -0x10(%rbp),%rax 4472c: c9 leaveq 4472d: c3 retq -------- Here, the 'sub' line appears to be allocating space on the stack for a local variable, presumeably s. But then it fails to initialize s before loading it into the return registers %rdx and %rax. I'm assuming that this is probably why the allocation of the local variable never got elided, as it was in the struct case -- the optimizer doesn't see the missing initialization of s, so it cannot elide the loads from the stack. So it looks like the bug is caused by failure to initialize s when generating code for a union vs. a struct. Why, though, is a mystery to me, since toString never actually references `this`, so in theory whether `this` is a struct or union shouldn't matter.
Comment #2 by hsteoh — 2018-01-12T18:25:07Z
Note also, the name `toString` is irrelevant to this bug. The same codegen bug appears if you rename the method to something else, like 'member'. Furthermore, the local variable doesn't have to be a string; any local variable without an initializer would also exhibit the same missing initialization, it doesn't initialize the local to .init, as it should.
Comment #3 by hsteoh — 2018-01-12T18:41:05Z
Minimized code: ------ union U { int method() { int x; return x; } } ------ The disassembly shows that x is never initialized to 0, and a garbage value is returned.
Comment #4 by hsteoh — 2018-01-12T19:44:19Z
More interesting clues: running a union method inside CTFE containing a local variable without an explicit initializer causes a CTFE error "cannot modify variable at compile time", whereas explicitly initializing the local variable works. ------ union U { int method() { int x; // causes CTFE failure unless explicitly initialized return x; } } enum y = U.init.method(); ------
Comment #5 by hsteoh — 2018-01-12T20:25:39Z
Comment #6 by github-bugzilla — 2018-01-15T06:25:44Z
Commits pushed to master at https://github.com/dlang/dmd https://github.com/dlang/dmd/commit/29c1e309be359f871d4052c025f7f3144005d092 Fix issue 18232: local vars in union member functions needs initializer. The `isunion` condition is too broad, because it also excludes local variables declared inside union member functions. Moreoever, it is not necessary, because the `fd` condition later on already excludes union fields, since they would not be inside a function declaration. https://github.com/dlang/dmd/commit/4cf228eb33ee8e2b4b6856fa7c08990217039ac7 Merge pull request #7687 from quickfur/issue18232 Fix issue 18232: local vars in union member functions needs initializer. merged-on-behalf-of: Sebastian Wilzbach <[email protected]>