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.

Sign up for GitHub

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

Jump to bottom

Fix issue 20339 - recalculate isPOD at the end of semantic analysis #10519

Closed
wants to merge 1 commit into from
Closed

Fix issue 20339 - recalculate isPOD at the end of semantic analysis #10519

wants to merge 1 commit into from

Conversation

SSoulaimane
Copy link
Member

@SSoulaimane SSoulaimane commented Oct 30, 2019 •
edited

This issue affects ABI with regards to argument passing.

SSoulaimane added a commit to SSoulaimane/dmd that referenced this pull request Oct 30, 2019
@SSoulaimane
Defer isPOD till end of semantic analysis from PR dlang#10519
1a89e5c 
This issue affects this work too. non POD structs have to be passed by ref for function arguments.
@SSoulaimane SSoulaimane force-pushed the fixpodcheck branch 4 times, most recently from df2888f to 52d9a8a Compare October 30, 2019 22:41
SSoulaimane added a commit to SSoulaimane/dmd that referenced this pull request Oct 30, 2019
@SSoulaimane
Defer isPOD till end of semantic analysis from PR dlang#10519
ed47a67 
This issue affects this work too. non POD structs have to be passed by ref for function arguments.
@SSoulaimane SSoulaimane changed the title Fix issue 20339 - recalculate isPOD at the end sematic analysis Fix issue 20339 - recalculate isPOD at the end semantic analysis Oct 30, 2019
@SSoulaimane SSoulaimane marked this pull request as ready for review October 30, 2019 23:25
@dlang-bot
Copy link
Contributor

dlang-bot commented Oct 30, 2019 •
edited

Thanks for your pull request and interest in making D better, @SSoulaimane! 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
✓ 20339 normal isPOD returns true if sizeof is accessed inside struct declaration

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 "master + dmd#10519"

@SSoulaimane SSoulaimane changed the title Fix issue 20339 - recalculate isPOD at the end semantic analysis Fix issue 20339 - recalculate isPOD at the end of semantic analysis Oct 31, 2019
PetarKirov
PetarKirov approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like another reviewer to also take look.

@thewilsonator
Copy link
Contributor

Please match the pragma message as expected output.

@SSoulaimane SSoulaimane force-pushed the fixpodcheck branch 3 times, most recently from 11b3663 to cd12293 Compare October 31, 2019 16:13
SSoulaimane added a commit to SSoulaimane/dmd that referenced this pull request Oct 31, 2019
@SSoulaimane
Defer isPOD till end of semantic analysis from PR dlang#10519
1243fdb 
This issue affects this work too. non POD structs have to be passed by ref for function arguments.
SSoulaimane added a commit to SSoulaimane/dmd that referenced this pull request Nov 1, 2019
@SSoulaimane
Defer isPOD till end of semantic analysis from PR dlang#10519
8c418a1 
This issue affects this work too. non POD structs have to be passed by ref for function arguments.
WalterBright
WalterBright reviewed Nov 1, 2019
View reviewed changes
src/dmd/dsymbolsem.d Outdated
@@ -4751,6 +4751,9 @@ private extern(C++) final class DsymbolSemanticVisitor : Visitor
if (sd.inv)
reinforceInvariant(sd, sc2);

// recalculate isPOD
sd.ispod = StructPOD.fwd;

Copy link
Member

Choose a reason for hiding this comment

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

This is a bit disturbing, as it suggests that isPOD can return different values for the same struct depending on where in the semantic analysis it is.

The most reliable way to deal with this is to have isPOD fail if semantic analysis has not proceeded far enough to get an accurate answer. Next best would be to have isPOD fail if it produces different results as semantic analysis proceeds. Worst would be to silently produce different results.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does actually return different results in the test case I provided.

SSoulaimane added a commit to SSoulaimane/dmd that referenced this pull request Nov 1, 2019
@SSoulaimane
Defer isPOD till end of semantic analysis from PR dlang#10519
b81057c 
This issue affects this work too. non POD structs have to be passed by ref for function arguments.
@SSoulaimane SSoulaimane force-pushed the fixpodcheck branch 6 times, most recently from 4e9d744 to c615462 Compare November 4, 2019 23:21
@SSoulaimane SSoulaimane force-pushed the fixpodcheck branch 3 times, most recently from ccc1565 to b4a5609 Compare November 7, 2019 23:25
@SSoulaimane SSoulaimane force-pushed the fixpodcheck branch 3 times, most recently from 0325d2d to 8ff06d4 Compare November 12, 2019 23:14
@SSoulaimane
Fix issue 20339 - Eagerly run a selective semantic pass for members w…
8ff06d4 
…hich affect the result of `isPOD`

If `isPOD` is called on a struct in the middle of the semantic analysis of the same srtuct; then pause the semantic analysis of the struct and eagerly process fields, constructors, destructors, and postblits, because they affect the result of `isPOD`, then determine if the struct is a POD and resume semantic analysis of the struct.

Sometimes members of a struct rely on the result of isPOD, it happens currently in phobos. `isPOD` is required to know if a function returns on stack, and is sometimes invoked during semantic analysis of a lambda function for `typeof()` or `__traits(compiles)`.
@RazvanN7
Copy link
Contributor

Closing due to inactivity. The original bugzilla report points to this PR in case someone else bumps into the issue and wants to fix it.

@RazvanN7 RazvanN7 closed this Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WalterBright WalterBright WalterBright left review comments

@PetarKirov PetarKirov PetarKirov approved these changes

@Geod24 Geod24 Awaiting requested review from Geod24 Geod24 is a code owner

@ibuclaw ibuclaw Awaiting requested review from ibuclaw ibuclaw is a code owner

Assignees
No one assigned
Labels
Bug Fix Enhancement Needs Rebase stalled
Projects
None yet
Milestone
No milestone
6 participants
@SSoulaimane @dlang-bot @thewilsonator @RazvanN7 @WalterBright @PetarKirov