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

Merged
merged 2 commits into from
Jan 20, 2019
Merged

Fix issue #2932 (allow speculative nested variables) #2940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
929c620
Fix issue #2932 (allow speculative nested variables)
kinke Dec 14, 2018
909e1a5
Safer variantQ
kinke Dec 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions gen/nested.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,27 @@ DValue *DtoNestedVariable(Loc &loc, Type *astype, VarDeclaration *vd,
IF_LOG { Logger::cout() << "Context: " << *ctx << '\n'; }

DtoCreateNestedContextType(vdparent->isFuncDeclaration());

assert(isIrLocalCreated(vd));
IrLocal *const irLocal = getIrLocal(vd);

// The variable may not actually be nested in a speculative context (e.g.,
// with `-allinst`, see https://github.com/ldc-developers/ldc/issues/2932).
// Use an invalid null storage in that case, so that accessing the var at
// runtime will cause a segfault.
if (irLocal->nestedIndex == -1) {
Logger::println(
"WARNING: isn't actually nested, using invalid null storage");
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

}

////////////////////////////////////
// Extract variable from nested context

assert(irfunc->frameType);
const auto frameType = LLPointerType::getUnqual(irfunc->frameType);
IF_LOG { Logger::cout() << "casting to: " << *irfunc->frameType << '\n'; }
LLValue *val = DtoBitCast(ctx, frameType);
Expand All @@ -108,7 +124,6 @@ DValue *DtoNestedVariable(Loc &loc, Type *astype, VarDeclaration *vd,
val = DtoAlignedLoad(val, name);
};

IrLocal *const irLocal = getIrLocal(vd);
const auto vardepth = irLocal->nestedDepth;
const auto funcdepth = irfunc->depth;

Expand All @@ -132,10 +147,7 @@ DValue *DtoNestedVariable(Loc &loc, Type *astype, VarDeclaration *vd,
IF_LOG Logger::cout() << "Frame: " << *val << '\n';
}

const auto idx = irLocal->nestedIndex;
assert(idx != -1 && "Nested context not yet resolved for variable.");

offsetToNthField(idx, vd->toChars());
offsetToNthField(irLocal->nestedIndex, vd->toChars());
IF_LOG {
Logger::cout() << "Addr: " << *val << '\n';
Logger::cout() << "of type: " << *val->getType() << '\n';
Expand Down
20 changes: 20 additions & 0 deletions tests/compilable/gh2932.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// RUN: %ldc -c -allinst %s

import std.algorithm;

extern __gshared int[] array;

void funcWithNoFrame()
{
int local;
// lambda is codegen'd
pragma(msg, typeof(array.map!(e => local)));
}

void funcWithFrame()
{
int capturedVar, local;
int nestedFunc() { return capturedVar; }
// lambda is codegen'd with `-allinst`
static assert(__traits(compiles, array.map!(e => local)));
}