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

Fix Issue 20415 - ice in dwarfeh and when optimizations are enabled #10630

Closed
wants to merge 1 commit into from

Conversation

RazvanN7
Copy link
Contributor

The assertion is triggered by a miscommunication between the frontend and the backend:

void t()
{
  auto a = false ? B().p : null;
}

struct B
{
  void* p;
  ~this() {}
}

The frontend fails to see that the first branch in the CondExp is never taken because it assigns the false to a temporary and cannot optimize it away so it thinks that it has to call the destructor of B if that branch is taken. Later the optimizer in the backend sees that the branch is never taken and it does not generate code for it, however it still tries to insert cond_result && temp.__dtor for the value supposedly constructed for the first branch. That's where the assert is coming from.

I fixed this issue, by taking care of this case in the frontend: if an IntegralExp is the condition, we no longer need to assign to a temporary, therefore the frontend optimizer can do its job.

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 29, 2019

Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
20415 normal ice in dwarfeh and when optimizations are enabled

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + dmd#10630"

@RazvanN7 RazvanN7 changed the base branch from master to stable November 29, 2019 18:48
@RazvanN7 RazvanN7 added the Regression PRs that fix regressions label Nov 29, 2019
@aliak00
Copy link

aliak00 commented Nov 29, 2019

Would a '\0' also as the condition also apply as an isIntergerExp1?

Also, when you add a void main() { t; } to the code, it doesn't crash. How come?

@rainers
Copy link
Member

rainers commented Nov 29, 2019

I don't see it failing on run.dlang.io since 2.083. If it still does for you, doesn't

bool c = false;
auto a = c ? B().p : null;

also trigger the ICE? That won't change with this PR.
The frontend doesn't have to do any optimizations, looks like a backend only bug according to your description.

if (!vcond)
// a temporary is created for the condition
// only if it's not an integral exp
if (!vcond && !ce.econd.isIntegerExp())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a bit simplistic. The optimizer is pretty smart, it is able to optimize array literal indexing, enums, const scalar variables and similar things.
The following will still segfault

bool ct() { return 1+1 == 5; }
void main()
{
    enum arr = [ct()];
    auto a = arr[1*0] ? B().p : null;
}
struct B
{
    void* p;
    ~this() {}
}

Since we don't want to run the optimizer here just yet you can just declare the vcond variable as const or immutable bool then the optimizer later on should be able to const fold it and eliminate the dead branch.
See

dmd/src/dmd/optimize.d

Lines 76 to 81 in ce96179

if (v.type &&
(v.isConst() || v.isImmutable() || v.storage_class & STC.manifest))
{
Type tb = v.type.toBasetype();
if (v.storage_class & STC.manifest ||
tb.isscalar() ||

if (!vcond)
// a temporary is created for the condition
// only if it's not an integral exp
if (!vcond && !ce.econd.isIntegerExp())
{
vcond = copyToTemp(STC.volatile_, "__cond", ce.econd);
vcond.dsymbolSemantic(sc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vcond.dsymbolSemantic(sc);
vcond.type = Type.tbool.immutableOf();
vcond.dsymbolSemantic(sc);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Regression PRs that fix regressions
Projects
None yet
5 participants