Bug 20844 – DMD compiler should take care of data alignment, after seeing the 'cas' call
Status
RESOLVED
Resolution
FIXED
Severity
enhancement
Priority
P1
Component
druntime
Product
D
Version
D2
Platform
x86_64
OS
Linux
Creation time
2020-05-19T16:13:33Z
Last change time
2020-05-26T07:06:50Z
Keywords
pull
Assigned to
No Owner
Creator
mw
Comments
Comment #0 by mingwu — 2020-05-19T16:13:33Z
(This is a follow up of: https://issues.dlang.org/show_bug.cgi?id=20838#c12)
I find a segfault with LDC: with this code on https://d.godbolt.org/z/HesA24
i.e. remove the import std.stdio and writeln
--------------------------------------------------------------------------------
$ cat c.d
//import std.stdio;
import core.atomic;
struct N {
N* prev;
N* next;
}
shared(N) n;
void main() {
cas(&n, n, n);
//writeln(size_t.sizeof*2, N.sizeof);
}
$ ldc2 -m64 c.d
$ ./c
Segmentation fault (core dumped)
$ ldc2 --version
LDC - the LLVM D compiler (1.20.0):
based on DMD v2.090.1 and LLVM 9.0.1
built with LDC - the LLVM D compiler (1.20.0)
Default target: x86_64-unknown-linux-gnu
Host CPU: skylake
http://dlang.org - http://wiki.dlang.org/LDC
--------------------------------------------------------------------------------
With import std.stdio and writeln, the LDC output behave normally (no segfault):
--------------------------------------------------------------------------------
$ ldc2 -m64 c.d
$ ./c
1616
--------------------------------------------------------------------------------
Currently we need to manually take care of required 16-bytes alignment:
align(16) shared(N) n; // or `align(2 * size_t.sizeof)`
Can the DMD compiler (after seeing the 'cas' call) take care of this alignment? either silently, or issue an warning message to the programmer?
The current behavior that I just discovered is definitely a puzzle for a D newbie like me. With a smarter compiler, it will help new users.
The druntime library is supposed to take care of this already, at least with enabled contracts, see https://github.com/dlang/druntime/blob/48082ac4e4aa1a3c9f1a1ef87659c941dae0f7f6/src/core/atomic.d#L624-L655. It only checks for insufficient size_t alignment though, so that needs to be fixed.
Wrt. original DMD issue here, DMD is supposed to use cmpxchg16b already, see https://github.com/dlang/druntime/blob/48082ac4e4aa1a3c9f1a1ef87659c941dae0f7f6/src/core/internal/atomic.d#L582. As it apparently doesn't, I guess the bug is in DMD's codegen.
Comment #1 by dlang-bot — 2020-05-19T21:22:27Z
@kinke created dlang/druntime pull request #3109 "Fix Issue 20844 - Insufficient alignment check for 16-bytes CAS on x86_64" fixing this issue:
- Fix Issue 20844 - Insufficient alignment check for 16-bytes CAS on x86_64
https://github.com/dlang/druntime/pull/3109
Comment #2 by dlang-bot — 2020-05-26T07:06:50Z
dlang/druntime pull request #3109 "Fix Issue 20844 - Insufficient alignment check for 16-bytes CAS on x86_64" was merged into master:
- 1e50000f3691b2b0e616dc383d792b1537e072df by Martin Kinkelin:
Fix Issue 20844 - Insufficient alignment check for 16-bytes CAS on x86_64
https://github.com/dlang/druntime/pull/3109