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 assert; arguments were in reverse order #16515

Merged
merged 1 commit into from
May 23, 2024

Conversation

Connor-GH
Copy link

So sometime between glibc 2.35 and glibc 2.38, assert for betterC stopped working.
After a wild goosechase, it was determined that not only was the wrong function being
called ("__assert" instead of "__assert_fail"), but the arguments were being passed to
el_bin backwards! The fix for this is obviously to fix the order of the arguments, and
this is reflected in the generated code. Musl was untested for this, but the fix should
not affect otherwise working musl systems due to version(CRuntime_Musl).

@dlang-bot
Copy link
Contributor

dlang-bot commented May 20, 2024

Thanks for your pull request and interest in making D better, @Connor-GH! 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
24558 normal C asserts segfault on Glibc

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#16515"

@Connor-GH Connor-GH closed this May 21, 2024
@Connor-GH Connor-GH reopened this May 21, 2024
@Connor-GH
Copy link
Author

Whoops, I accidently closed this.

@schveiguy
Copy link
Member

schveiguy commented May 21, 2024

This needs a test. And I suspect it might be dependent on the glibc version.

In discord chat, we found that on my glibc 2.35 version, it worked, but on @Connor-GH 's glibc 2.38 version it had a segfault.

If this is the case, it's going to be a tricky thing to get right.

Edit: Still needs a test, but I'm realizing this is a different function being called! It's __assert_fail and not __assert, which we agreed is the correct call.

compiler/src/dmd/e2ir.d Outdated Show resolved Hide resolved
compiler/src/dmd/e2ir.d Outdated Show resolved Hide resolved
compiler/src/dmd/e2ir.d Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented May 21, 2024

This needs a test. And I suspect it might be dependent on the glibc version.

Assuming that there aren't any per-arch overrides, implementation hasn't changed since 2002.

https://github.com/bminor/glibc/blob/f83e461f1014598a5cb4c89407ce303b9f0bd8ac/assert/__assert.c#L23

In discord chat, we found that on my glibc 2.35 version, it worked, but on @Connor-GH 's glibc 2.38 version it had a segfault.

If this is the case, it's going to be a tricky thing to get right.

Edit: Still needs a test, but I'm realizing this is a different function being called! It's __assert_fail and not __assert, which we agreed is the correct call.

Arguably both are wrong because this is relying on glibc internals that are not intended to be called directly like this.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

we should be using irs.target.c.runtime here not version(Runtime_Foo)

if (irs.target.os == Target.OS.OSX) { ... }
else
{
    Symbol* assertSym;
    elem* params;
    with (TargetC.Runtime) switch  (irs.target.c.runtime)
    {
        case Musl:
            assertSym = getRtlsym(RTLSYM.C__ASSERT_FAIL);
            params = el_params(elmsg, eline, efilename, efunc, null);
            break;
        case GLibc: 
            //...
    }
    elem* efunc = getFuncName();
    auto eassert = el_var(assertSym);
    ea = el_bin(OPcall, TYvoid, eassert, params);
}

@Connor-GH
Copy link
Author

This needs a test. And I suspect it might be dependent on the glibc version.

Assuming that there aren't any per-arch overrides, implementation hasn't changed since 2002.

https://github.com/bminor/glibc/blob/f83e461f1014598a5cb4c89407ce303b9f0bd8ac/assert/__assert.c#L23

In discord chat, we found that on my glibc 2.35 version, it worked, but on @Connor-GH 's glibc 2.38 version it had a segfault.
If this is the case, it's going to be a tricky thing to get right.
Edit: Still needs a test, but I'm realizing this is a different function being called! It's __assert_fail and not __assert, which we agreed is the correct call.

Arguably both are wrong because this is relying on glibc internals that are not intended to be called directly like this.

I purposefully didn't touch the musl version as I did not test it. I would rather only touch code that I can test.

@Connor-GH
Copy link
Author

Connor-GH commented May 21, 2024

I'd also like to mention that the CI is pretty outdated. None of the CI machines run glibc 2.38.

Edit: even godbolt.org runs glibc 2.35.

@Connor-GH
Copy link
Author

Connor-GH commented May 21, 2024

Tested on void linux musl, the assert was backwards for musl too and that I was mistaken. The implementation correct for OSX as-is.

@Connor-GH
Copy link
Author

Connor-GH commented May 21, 2024

(This is after the latest patch which also fixes the musl version. Before it would also segfault)

2024-05-21-1158AM

@schveiguy
Copy link
Member

Arguably both are wrong because this is relying on glibc internals that are not intended to be called directly like this.

Agreed. It is something I was thinking importC might actually be better at solving. But of course, this then requires a C compiler for building anything.

compiler/src/dmd/e2ir.d Outdated Show resolved Hide resolved
Copy link
Author

@Connor-GH Connor-GH left a comment

Choose a reason for hiding this comment

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

I have implemented this.

@Connor-GH
Copy link
Author

Simply waiting on approval now for CI.

@thewilsonator
Copy link
Contributor

Is there a bugzilla issue number for this? if not please create one, or add a changelog entry for this. This otherwise looks good, the one failure I think will go away if you rebase this PR

@Connor-GH
Copy link
Author

Use irs.target.c.runtime instead of version(CRuntime_)

Unify musl and glibc implementation
@thewilsonator thewilsonator merged commit 90229ba into dlang:master May 23, 2024
40 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants