dmd
dmd copied to clipboard
dlang
Enable -checkaction=context by default
Putting this up there to see how people feel about it / what breaks. Currently this switch is not usable as it creates a linker error in Phobos. Having it the default would bring a lot of benefit - at the expense of breaking some code, e.g. non-copyable structs or structs with side effect.
Side note: That was the whole point of having -preview=in - make this kind of generic code easier to write.
Running it on my machine yields an ICE on some tests:
... runnable/structlit.d -preview=rvaluerefparam -fPIC (-inline -release -g -O)
==============================
Test 'runnable/structlit.d' failed. The logged output:
/Users/geod24/projects/dlang/dmd/generated/osx/release/64/dmd -conf= -m64 -Irunnable -preview=rvaluerefparam -fPIC -od/Users/geod24/projects/dlang/dmd/test/test_results/runnable -of/Users/geod24/projects/dlang/dmd/test/test_results/runnable/structlit_0 runnable/structlit.d
/Users/geod24/projects/dlang/dmd/test/test_results/runnable/structlit_0
Success
/Users/geod24/projects/dlang/dmd/generated/osx/release/64/dmd -conf= -m64 -Irunnable -preview=rvaluerefparam -fPIC -inline -od/Users/geod24/projects/dlang/dmd/test/test_results/runnable -of/Users/geod24/projects/dlang/dmd/test/test_results/runnable/structlit_1 runnable/structlit.d
Assertion failed: ((*s).Ssymnum == 18446744073709551615LU), function dmd.backend.symbol.symbol_add, file src/dmd/backend/symbol.d, line 1101.
==============================
Test 'runnable/structlit.d' failed: caught signal: -6
>>> TARGET FAILED: runnable/structlit.d
... runnable/inline.d -fPIC (-inline -release -g -O)
==============================
Test 'runnable/inline.d' failed. The logged output:
/Users/geod24/projects/dlang/dmd/generated/osx/release/64/dmd -conf= -m64 -Irunnable -fPIC -od/Users/geod24/projects/dlang/dmd/test/test_results/runnable -of/Users/geod24/projects/dlang/dmd/test/test_results/runnable/inline_0 runnable/inline.d runnable/imports/a14267.d
/Users/geod24/projects/dlang/dmd/test/test_results/runnable/inline_0
7
83
4
4
1.000000 2.000000 3.000000
Success
/Users/geod24/projects/dlang/dmd/generated/osx/release/64/dmd -conf= -m64 -Irunnable -fPIC -inline -od/Users/geod24/projects/dlang/dmd/test/test_results/runnable -of/Users/geod24/projects/dlang/dmd/test/test_results/runnable/inline_1 runnable/inline.d runnable/imports/a14267.d
Assertion failed: ((*s).Ssymnum == 18446744073709551615LU), function dmd.backend.symbol.symbol_add, file src/dmd/backend/symbol.d, line 1101.
Also a deprecation triggering here and there:
/Users/geod24/projects/dlang/dmd/test/../../druntime/import/core/internal/dassert.d(130): Deprecation: argument `v` for format specification `"%u"` must be `int`, not `const(ulong)`
e.g. non-copyable structs or structs with side effect.
Those should already work, could you provide an example that fails?
There's also another problem with DIP1000 flagging the hidden temporaries created by -checkaction=context that needs to be fixed first.
The backend assertion failures seems to be related to the inliner creating a really weird AST:
Test case (which passes without -inline):
// REQUIRED_ARGS: -checkaction=context -inline
struct S
{
S get()() const { return this; }
}
auto f(S s) { return s; }
void main()
{
auto s = S();
assert(f(s.get) == s);
}
AST without -inline:
void main()
{
S s = S();
assert(f(s.get()) is s, _d_assert_fail(f(s.get()), s));
return 0;
}
With -inline:
void main()
{
S s = S();
assert(((S s = s;) , (s)) is s, ((const(S) a = (S s = s;) , (s);) , ((ref const(S) b = s;)) , ((string valA = (ref const(S) t = a;) , (alias miniT = pure nothrow @safe string miniFormat(ref const(S) v)
{
import core.internal.traits : isAggregateType;
import core.stdc.stdio : sprintf;
import core.stdc.string : strlen;
string msg = "S(";
/*unrolled*/ {
}
msg ~= ")";
return msg;
}
;
, (*((string function(ref const(S)) pure nothrow @safe t = & miniFormat;) , (alias RT = string;
, (alias P = (ref const(S));
) , (alias type = string function(ref const(S)) pure nothrow @nogc @safe;
) , cast(string function(ref const(S)) pure nothrow @nogc @safe)t)))(t));) , ((string valB = (ref const(S) t = b;) , (alias miniT = pure nothrow @safe string miniFormat(ref const(S) v)
{
import core.internal.traits : isAggregateType;
import core.stdc.stdio : sprintf;
import core.stdc.string : strlen;
string msg = "S(";
/*unrolled*/ {
}
msg ~= ")";
return msg;
}
;
, (*((string function(ref const(S)) pure nothrow @safe t = & miniFormat;) , (alias RT = string;
, (alias P = (ref const(S));
) , (alias type = string function(ref const(S)) pure nothrow @nogc @safe;
) , cast(string function(ref const(S)) pure nothrow @nogc @safe)t)))(t));)) , ((enum string token = "!=";)) , combine(valA, "!=", valB))));
return 0;
}
Or is that just -vcg-ast producing weird output?
It fails on tupleof. https://github.com/dlang/dmd/pull/11987 will help with debugging.
The backend assertion failures seems to be related to the inliner creating a really weird AST:
https://issues.dlang.org/show_bug.cgi?id=21471
It fails on tupleof. #11987 will help with debugging.
https://issues.dlang.org/show_bug.cgi?id=21472
Hopefully. I don't know what's causing the backend issue and have no plan to look into it in the foreseeable future. However @MoonlightSentinel and I did look into the tuple ones, and he submitted a couple draft PRs, so I have good hope this one will be fixed soon-ish.
@MoonlightSentinel : Looks like the fix for tuple was incomplete:
Test 'runnable/aliasthis.d' failed. The logged output:
/tmp/cirrus-ci-build/generated/linux/release/64/dmd -conf= -m64 -Irunnable -fPIC -odtest_results/runnable -oftest_results/runnable/aliasthis_0 runnable/aliasthis.d
false
[] = int
[] = string
[0] = int
[1] = string
[] = string
[] = int
[1] = string
[0] = int
runnable/aliasthis.d(1473): Error: incompatible types for `(__assertOp502) == (__assertOp504)`: both operands are of type `()`
runnable/aliasthis.d(1541): Error: incompatible types for `(__assertOp508) == (__assertOp510)`: both operands are of type `(int, long)`
==============================
Code in question:
void test10178()
{
struct S { static int count; }
S s;
assert((s.tupleof == s.tupleof) == true);
assert((s.tupleof != s.tupleof) == false);
S getS()
{
S s;
++S.count;
return s;
}
assert(getS().tupleof == getS().tupleof); // L1473
assert(S.count == 2);
}
void test10004()
{
static int count = 0;
static S make(S)()
{
++count; // necessary to make this function impure
S s;
return s;
}
struct SX(T...) {
T field; alias field this;
}
alias S = SX!(int, long);
assert(make!S.field == make!S.field); // L1541
assert(count == 2);
}
And it trips on the following code:
assert(( boo(4) = 2) == 2); assert(( maz(4) = 2) == 2);
assert((4.boo = 2) == 2); assert((4.maz = 2) == 2);
assert(( coo(a) = 2) == 2); assert(( maz(a) = 2) == 2);
assert((a.coo = 2) == 2); assert((a.maz = 2) == 2);
assert(( mar(s) = 2) == 2); assert(( maz(s) = 2) == 2);
assert((s.mar = 2) == 2); assert((s.maz = 2) == 2);
@MoonlightSentinel : Looks like the fix for tuple was incomplete:
Reduced example:
struct S
{
int a, b;
}
S get();
alias AliasSeq(T...) = T;
void main()
{
assert(get().tupleof == AliasSeq!(1, 2));
}
Seems like there is some magic w.r.t. to comparing tuple variables. I'll have a look.
Looks like the rewrite still doesn't understand:
assert(( boo(4) = 2) == 2); assert(( maz(4) = 2) == 2);
The druntime failure is ... weird:
static assert(!__traits(compiles,
() @safe {
struct BadValue
{
int x;
this(this) @safe { *(cast(ubyte*)(null) + 100000) = 5; } // not @safe
alias x this;
}
BadValue[int] aa;
() @safe { auto x = aa.byKey.front; } ();
}
));
void issue15367()
{
void delegate() pure nothrow @nogc @safe dg;
assert(dg == dg);
}
Remove the __traits(compiles, ...) or @safe and it works as expected. Looks like some failure leaks out of the conditional compilation...
(This is probably unrelated to -checkaction=context)
... runnable/previewin.d -preview=dip1000 -preview=in -fPIC ()
==============================
Test 'runnable/previewin.d' failed. The logged output:
/home/circleci/dmd/generated/linux/debug/64/dmd -conf= -m64 -Irunnable -preview=dip1000 -preview=in -fPIC -od/home/circleci/dmd/test/test_results/runnable -of/home/circleci/dmd/test/test_results/runnable/previewin_0.o -c runnable/previewin.d
/home/circleci/dmd/test/../../druntime/import/core/internal/dassert.d(335): Error: copy constructor `previewin.NonCopyable.this` cannot be used because it is annotated with `@disable`
/home/circleci/dmd/test/../../druntime/import/core/internal/dassert.d(414): Error: template instance `core.internal.dassert.miniFormat!(const(NonCopyable)[NonCopyable])` error instantiating
/home/circleci/dmd/test/../../druntime/import/core/internal/dassert.d(67): instantiated from here: `miniFormatFakeAttributes!(const(NonCopyable)[NonCopyable])`
runnable/previewin.d(86): instantiated from here: `_d_assert_fail!(NonCopyable[NonCopyable])`
Damn I really love this test. https://github.com/dlang/druntime/pull/3412
Testing profile
./generated/linux/release/64/profile ./generated/linux/release/64/mytrace.log ./generated/linux/release/64/mytrace.def
grep -q '1 .*_Dmain' ./generated/linux/release/64/mytrace.log
grep -q '1000 .*uint profile.foo(uint)' ./generated/linux/release/64/mytrace.log
diff mytrace.def.exp ./generated/linux/release/64/mytrace.def || diff mytrace.releaseopt.def.exp ./generated/linux/release/64/mytrace.def
2a3
> _D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
5d5
< _D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
3a4
> _D4core8internal5array10comparison__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi
Not sure what to make of this yet. Test introduced in https://github.com/dlang/druntime/pull/2467
Not sure what to make of this yet. Test introduced in dlang/druntime#2467
Looks like diff fails because this patch changes the order of functions listed in mytrace.def.
Guessing from the diff: Before
FUNCTIONS
_D4core8internal5array10comparison__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi
_Dmain
_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
_D7profile3fooFkZk
After
FUNCTIONS
_Dmain
_D4core8internal6string__T7dstrcmpZQjFNaNbNiNeMxAaMxQeZi
_D4core8internal5array10comparison__T5__cmpTaZQjFNaNbNiNeMxAaMxQeZi
_D7profile3fooFkZk
EDIT: Not sure if this is the actual error. Removing the outdated .def in dlang/druntime#3413
EDIT 2: The order changed, fixed in dlang/druntime#3415
What causes these weird linker errors on OSX?
ld: warning: direct access in function '__D10testtypeid5test4FZv'
from file 'test_results/runnable/testtypeid_1.o'
to global weak symbol '__D11TypeInfo_Af6__initZ'
from file '/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/phobos/generated/osx/release/64/libphobos2.a(typeinfo_bf1_434.o)'
means the weak symbol cannot be overridden at runtime.
This was likely caused by different translation units being compiled with different visibility settings.
Thanks for your pull request, @Geod24!
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 run digger -- build "master + dmd#11925"
@Geod24 @MoonlightSentinel I haven't been following up lately with all the progress on -checkaction=context, so can you guys give an update?
Some changes from the implementation PoV:
- supports tuples
- works during CTFE
- aborts when called from a finalizer (supplementary to
AssertErrorusingstaticError) - minor bugfixes w.r.t. nested types, overlapped fields, ...
Remaining problems:
- linker errors because DMD skips codegen for template instances expected from druntime / phobos - which are not compiled with
-checkaction=context(-allinstmight work though) - deprecated types used in
asserts are problematic due to https://issues.dlang.org/show_bug.cgi?id=7619 - ?
TL,DR: Formatting should work for most assert's except complex expressions using && / ||
We've been using it for our project for a while, and it works well. The linker errors are the only issues we've seen from time to time. I think we should turn it on in druntime / Phobos for the next release so that people can more widely use it.
On the deprecation side of things, we've had those for a while:
agora 0.15.0+commit.4.ga7ef19d7e: building configuration "unittest"...
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(295,30): Deprecation: function `vibe.core.path.GenericPath!(PosixPathFormat).GenericPath.Segment.toString` is deprecated - Use .name instead.
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(428,19): instantiated from here: `miniFormat!(Segment)`
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(67,49): instantiated from here: `miniFormatFakeAttributes!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(371,11): instantiated from here: `_d_assert_fail!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(197,19): instantiated from here: `GenericPath!(PosixPathFormat)`
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(295,30): Deprecation: function `vibe.core.path.GenericPath!(InetPathFormat).GenericPath.Segment.toString` is deprecated - Use .name instead.
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(428,19): instantiated from here: `miniFormat!(Segment)`
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(67,49): instantiated from here: `miniFormatFakeAttributes!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(371,11): instantiated from here: `_d_assert_fail!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(200,18): instantiated from here: `GenericPath!(InetPathFormat)`
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(295,30): Deprecation: function `vibe.core.path.GenericPath!(WindowsPathFormat).GenericPath.Segment.toString` is deprecated - Use .name instead.
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(428,19): instantiated from here: `miniFormat!(Segment)`
/Users/geod24/dlang/ldc-1.26.0/bin/../import/core/internal/dassert.d(67,49): instantiated from here: `miniFormatFakeAttributes!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(371,11): instantiated from here: `_d_assert_fail!(Segment)`
submodules/vibe-core/source/vibe/core/path.d(194,21): instantiated from here: `GenericPath!(WindowsPathFormat)`
Not sure how to best approach those.
Hum, this fails now because _d_assert_fail is not pure ~, I assume since the introduction of inFinalizer :/~
Will look more into it.
We've been using it for our project for a while, and it works well. The linker errors are the only issues we've seen from time to time. I think we should turn it on in druntime / Phobos for the next release so that people can more widely use it.
I'm not sure whether building Druntime + Phobos with -checkaction=context will actually be an improvement given that they also use -release and hence never instantiate the required templates. It would also be prone to fail as other external libraries are involved.
Two possible solutions:
- Only generate
_d_assert_failcalls in root modules. This ensures that the template emission code cannot assume existing template instances in druntime/phobos/because those modules won't call _d_assert_fail. THe only drawback is that this also disables message formatting for those modules during CTFE. - Always emit
_d_assert_failand other templates fromcore.internal.dassertThis prevents missing symbols but will probably generate a lot of duplicate instances and hence increase build times.
Only generate
_d_assert_failcalls in root modules.
Isn't that currently the case ?
On the deprecation side of things, we've had those for a while:
Guess we could check for deprecated toString - not sure if that's to much special casing