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

[REG2.067a] Issue 13999 - Associative array literal with static array keys must now have matching key lengths #4307

Merged
merged 2 commits into from Jan 19, 2015

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Jan 18, 2015

https://issues.dlang.org/show_bug.cgi?id=13999

Re-enable implicit length extension, eg. "abc" to char[5].

@9rnsr 9rnsr changed the title Issue 13999 - Associative array literal with static array keys must now have matching key lengths [REG2.067a] Issue 13999 - Associative array literal with static array keys must now have matching key lengths Jan 18, 2015
@9rnsr 9rnsr force-pushed the fix13999 branch 2 times, most recently from 1f54053 to f3e2a5d Compare January 18, 2015 12:47
void test13907()
{
static assert(!__traits(compiles, { f13907_1("\U00010000"w); }));
static assert(!__traits(compiles, { f13907_1("\U00010000" ); }));
f13907_2("\U00010000"w);
f13907_2("\U00010000");
static assert(!__traits(compiles, { f13907_3("\U00010000"w); }));
static assert(!__traits(compiles, { f13907_3("\U00010000" ); }));
//f13907_3("\U00010000"w); // Re-enable implicit length extension, from issue 13999 (unlisted codegen bug)
Copy link
Member

Choose a reason for hiding this comment

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

What are the details of the codegen bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line segfaults at runtime.

Copy link
Member

Choose a reason for hiding this comment

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

Ok I'll take a look.

Copy link
Member

Choose a reason for hiding this comment

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

How do you produce the segfault? What platform/args? I'm not seeing it on win32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens in win32 platform, without any args. Test case:

void f13907_3(wchar[3] a) {}
void main()
{
    f13907_3("\U00010000"w);
}

Note that, git-head fails to compile the code. To reproduce runtime segfault, you need to use 2.066 or cherry-pick this PR.

Copy link
Member

Choose a reason for hiding this comment

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

The error appears to happen because the string literal has type wchar[3] but se->len is set to 2.

wchar[2] can be passed in a single 32-bit stack slot, while wchar[3] requires two. As the calling convention uses callee-cleanup, the mismatch in allocated stack space causes a crash when the caller allocates only 4 bytes, and the callee frees 8, then loads the return address from the wrong offset.

I can see a couple of ways to deal with this:

  1. Update len when an implicit cast to T[N] is performed (I think something similar is done when character width changes?)
  2. Change e2ir.c:1277 to pass the static array's dim to toStringSymbol instead of se->len (there may be other places that need to be updated)

I think 1 is probably the safest option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I found and fixed a bug in StringExp::castTo().

…st now have matching key lengths

Re-enable implicit length extension, eg. "abc" to char[5]
@yebblies
Copy link
Member

Auto-merge toggled on

yebblies added a commit that referenced this pull request Jan 19, 2015
[REG2.067a] Issue 13999 - Associative array literal with static array keys must now have matching key lengths
@yebblies yebblies merged commit 356d52f into dlang:master Jan 19, 2015
@9rnsr 9rnsr deleted the fix13999 branch January 19, 2015 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants