Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable -checkaction=context by default #11925

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Nov 5, 2020

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.

@Geod24
Copy link
Member Author

Geod24 commented Nov 5, 2020

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)`

@MoonlightSentinel
Copy link
Contributor

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.

@MoonlightSentinel
Copy link
Contributor

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?

@WalterBright
Copy link
Member

Each of these failures needs to be posted to bugzilla.

@Geod24
Copy link
Member Author

Geod24 commented Nov 23, 2020

It fails on tupleof. #11987 will help with debugging.

@Geod24
Copy link
Member Author

Geod24 commented Dec 11, 2020

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

@Geod24
Copy link
Member Author

Geod24 commented Dec 11, 2020

It fails on tupleof. #11987 will help with debugging.

https://issues.dlang.org/show_bug.cgi?id=21472

@RazvanN7
Copy link
Contributor

RazvanN7 commented Jan 7, 2021

@Geod24 would fixing those 2 issues make this PR green?

@Geod24
Copy link
Member Author

Geod24 commented Jan 7, 2021

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.

@Geod24
Copy link
Member Author

Geod24 commented Feb 18, 2021

Rebased on master, let's see how this fare.

@Geod24
Copy link
Member Author

Geod24 commented Feb 18, 2021

@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);
}

@Geod24
Copy link
Member Author

Geod24 commented Feb 18, 2021

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
Copy link
Contributor

@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.

@MoonlightSentinel
Copy link
Contributor

(Rebased to restart the test suite now that #12258 was merged)

@Geod24
Copy link
Member Author

Geod24 commented Mar 22, 2021

Looks like the rewrite still doesn't understand:

    assert((  boo(4) = 2) == 2);    assert((  maz(4) = 2) == 2);

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Mar 22, 2021

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)

@Geod24
Copy link
Member Author

Geod24 commented Mar 25, 2021

 ... 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. dlang/druntime#3412

@Geod24
Copy link
Member Author

Geod24 commented Mar 25, 2021

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 dlang/druntime#2467

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Mar 25, 2021

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

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Mar 26, 2021

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.

@MoonlightSentinel MoonlightSentinel force-pushed the checkaction-default branch 2 times, most recently from 13c22bd to e017ed6 Compare March 26, 2021 23:52
@Geod24
Copy link
Member Author

Geod24 commented Jul 28, 2021

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.

@Geod24
Copy link
Member Author

Geod24 commented Oct 19, 2021

Hum, this fails now because _d_assert_fail is not pure , I assume since the introduction of inFinalizer :/
Will look more into it.

@Geod24 Geod24 force-pushed the checkaction-default branch 2 times, most recently from ac55335 to 8d4baa9 Compare October 19, 2021 08:56
@MoonlightSentinel
Copy link
Contributor

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:

  1. Only generate _d_assert_fail calls 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.
  2. Always emit _d_assert_fail and other templates from core.internal.dassert
    This prevents missing symbols but will probably generate a lot of duplicate instances and hence increase build times.

@Geod24
Copy link
Member Author

Geod24 commented Oct 19, 2021

Only generate _d_assert_fail calls in root modules.

Isn't that currently the case ?

@MoonlightSentinel
Copy link
Contributor

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

@MoonlightSentinel
Copy link
Contributor

Isn't that currently the case ?

Kindof. The call is emitted iff we run semantic on the function body - which includes auto functions and template instances in non-root modules AFAIU.

@Geod24
Copy link
Member Author

Geod24 commented Oct 19, 2021

So after a couple fixes:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants