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 #2932 (allow speculative nested variables) #2940

Merged
merged 2 commits into from
Jan 20, 2019

Conversation

kinke
Copy link
Member

@kinke kinke commented Dec 14, 2018

No description provided.

gen/nested.cpp Outdated
@@ -84,11 +100,11 @@ DValue *DtoNestedVariable(Loc &loc, Type *astype, VarDeclaration *vd,
IF_LOG { Logger::cout() << "Context: " << *ctx << '\n'; }

DtoCreateNestedContextType(vdparent->isFuncDeclaration());
assert(isIrLocalCreated(vd));
Copy link
Member Author

Choose a reason for hiding this comment

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

@klickverbot: I guess it would be safer to do the check after the DtoCreateNestedContextType() call?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, it's been a while – I'd need to look into this myself.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, I changed it to the safe variant.

auto llType = DtoPtrToType(astype);
if (isSpecialRefVar(vd))
llType = llType->getPointerTo();
return makeVarDValue(astype, vd, llvm::ConstantPointerNull::get(llType));
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure as to whether null or undefined makes more sense here.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't expect it to be used (or only from unused code), I'd use undef.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh yeah, those functions shouldn't be called, but we could just as well have a bug somewhere leading to this, where we wouldn't bail out anymore. I now lean more towards null for deterministic segfaults; e.g., the std.random 2.084 issue (undef initial seed) was only noticeable on 32-bit Linux.

Copy link
Member

Choose a reason for hiding this comment

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

Using null would not guarantee segfaults, but it clearly defines accessing it as UB (unless with "null-pointer-is-valid"=true, but that would then result in segfaulting). I don't think that dereferencing undef is UB in LLVM IR. So I too think null is the better choice here (optimizer can perhaps also use it e.g to remove dead code).

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense

@kinke
Copy link
Member Author

kinke commented Jan 17, 2019

Are we okay with this? We probably all agree that such functions shouldn't be emitted in the first place, but in the meantime, this hopefully allows people to play around with -allinst as workaround for linker errors due to template culling (last time I suggested this to someone, he replied that LDC immediately crashed, and I bet due to this).

@kinke kinke merged commit 61ed056 into ldc-developers:master Jan 20, 2019
@kinke kinke deleted the fix2932 branch January 20, 2019 13:59
@kinke kinke mentioned this pull request Jan 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants