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

testgc2: Make sure GC allocations are not optimized out. #1191

Merged
merged 2 commits into from
Nov 19, 2012

Conversation

dnadlinger
Copy link
Member

This makes sure the GC allocations are not optimized out, causing the assert to be hit, in addition to some cosmetic changes in a separate commit.

@dnadlinger dnadlinger closed this Oct 17, 2012
@dnadlinger dnadlinger reopened this Oct 17, 2012
@dnadlinger
Copy link
Member Author

Now just using a global variable. This is enough for LDC without link-time optimizations turned on to not optimize the allocations away, but of course not an ideal solution. If anybody has a better idea, please let me know.

@alexrp
Copy link
Member

alexrp commented Oct 20, 2012

Seems OK to me.

assert(0);
dummy = new byte[size_t.max / 3];
version (Windows)
assert(0);
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be Windows only, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, I have no idea why it is in there, so I just didn't touch it. Speculation mode on: Allocating size_t.max / 3 bytes might be possible on some 32 bit systems if they provide an uncluttered address space to applications, but maybe this is guaranteed to not happen on Windows?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, now you shed some light on this. Windows has only 3GB address space for 32 bit applications.

@MartinNowak
Copy link
Member

This is enough for LDC without link-time optimizations turned on to not optimize the allocations away, but of course not an ideal solution. If anybody has a better idea, please let me know.

If it's not an actual problem then it's good for now. If LTO is really breaking this you could try to make it a shared variable and/or use atomicStore/asm.

@alexrp
Copy link
Member

alexrp commented Oct 20, 2012

You know, on second thought...

Why does this test even exist? Shouldn't it be in druntime, if anything? And even then, don't we have plenty of code virtually everywhere that tests this stuff? And besides, maintaining it seems to turn into a battle against the compiler's optimizer(s) which may behave differently over time.

Or am I being silly?

@donc
Copy link
Collaborator

donc commented Oct 23, 2012

No, you're not being silly. Some of the tests in the test suite are very, very old (years older than D1.00), and date from long before the runtime even existed. Even now, there are very few tests in druntime. All these kind of tests definitely belong there. On the other hand, we currently have no mechanism for stand-alone tests for phobos and druntime.

@yebblies
Copy link
Member

Using the allocated data in a call to a runtime function (getCapacity?) would be a more robust way to prevent it from being optimized out.

@dnadlinger
Copy link
Member Author

Rebased and changed the code to just printf the capacities of the allocated chunks to avoid optimizations.

@dnadlinger
Copy link
Member Author

Ping?

@dnadlinger
Copy link
Member Author

Rebased again. Should be entirely uncontroversial now (well, in theory a clever compiler could just set capacity to 0 and remove the allocations anyway, but I doubt this will ever be worth it).

donc pushed a commit that referenced this pull request Nov 19, 2012
testgc2: Make sure GC allocations are not optimized out.
@donc donc merged commit 80dc1d6 into dlang:master Nov 19, 2012
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.

5 participants