Skip to content

Add memory error handler when requested via the#11646

Merged
wilzbach merged 1 commit intodlang:masterfrom
schveiguy:addmemerrhandler
Aug 30, 2020
Merged

Add memory error handler when requested via the#11646
wilzbach merged 1 commit intodlang:masterfrom
schveiguy:addmemerrhandler

Conversation

@schveiguy
Copy link
Copy Markdown
Member

DMD_INSTALL_MEMERR_HANDLER=1 environment variable. Only works on DMD,
and currently only on Linux.

See #11574 for more details.

I did not test the NoMain version, but I assume this should work since it should be compiled with the -main option.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request, @schveiguy!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#11646"

@schveiguy
Copy link
Copy Markdown
Member Author

ping @wilzbach @MoonlightSentinel

DMD_INSTALL_MEMERR_HANDLER=1 environment variable. Only works on DMD,
and currently only on Linux.
@wilzbach
Copy link
Copy Markdown
Contributor

Force-merging as auto-tester Windows machines have issues atm.

@wilzbach wilzbach merged commit 4d04802 into dlang:master Aug 30, 2020
}
else
{
printf("**WARNING** Memory error handler not supported on this platform!\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Printing this error message would require us to guess if the MEH is available for each CI machine (because it would break the TEST_OUTPUT validation).

This could be disabled via another env. variable but simply removing it should be acceptable as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we will be always enable this feature for all CI? I mean, I have definitely seen problems with this in some cases that made the code worse than not having it installed.

In which case, it's useful for using the CI to find issues (like the original #11574), or to use it locally when a segfault happens.

Perhaps I can change the code to configure the printout based on the value of the string? Like if the environment variable is not blank, then enable the memory handler, and if the value is not "NOPRINT", then print out the message?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't need to be enabled for all CI's. I thought we could e.g. enable it e.g. for the -debug builds done on Circle and Azure* (unless it causes errors, etc).

Perhaps I can change the code to configure the printout based on the value of the string? Like if the environment variable is not blank, then enable the memory handler, and if the value is not "NOPRINT", then print out the message?

Something like that, allthough it could simply use 1 for the current behaviour and 2 for silent mode (I doubt that the silent mode is ever relevant outside of our CI's)

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