Fix issue 17075 ctRegex BacktrackingMatcher.prevStack: free(): invali…

[DmitryOlshansky]

Backtracking engine memory management was horribly broken in multiple ways.
This fixes the mentioned issue and a couple other subtle bugs.

Finally I can pass the test suite with memory chunk size of 1 state, so things are more robust.

[dlang-bot]

Fix	Bugzilla	Description
✓	17075	ctRegex BacktrackingMatcher.prevStack: free(): invalid pointer

[DmitryOlshansky]

I don't get it - CircleCI keeps naging about this line - space after 'while'. But in the latest commit there IS a space between while and paren.

[DmitryOlshansky]

Overall - automation is pain in the ass when it doesn't work.

[JackStouffer]

Same stable problem will happen here

[DmitryOlshansky]

Ping @JackStouffer
CC @MartinNowak

This is important bugfix let's not allow it to slip through.

[quickfur]

I think it's complaining because it's expecting { to begin on a newline after while(...).

Maybe the circleCI script should be fixed to allow this if it's an empty block.

[DmitryOlshansky]

I think it's complaning because it's expecting { to begin on a newline after while(...).

Then it has to be fixed not the code. It makes no sense to put {} on the new line just to appease the script.

[quickfur]

I agree. But there's also these errors:

out/std_regex_package.d(43): Error: undefined identifier 'popFrontN'
out/std_regex_package.d(150): Error: undefined identifier 'Yes.keepSeparators'
out/std_regex_package.d(154): Error: undefined identifier 'Yes.keepSeparators'

[DmitryOlshansky]

I agree. But there's also these errors:

These existed on the stable branch and should be fixed separately if at all.

[aG0aep6G]

CircleCI clearly complains about this line, guys. Needs a space after while (and one before {}, probably).

[quickfur]

You're right, that's the real cause. Sorry for the noise.

[DmitryOlshansky]

Thanks @aG0aep6G, will fix.

[quickfur]

You're still missing a space between while and the opening (.

[quickfur]

Btw, what's the feature somebody mentioned where reviewers can edit the PR themselves? That would be very useful here instead of having so many roundtrips back and forth. How does that feature work?

[DmitryOlshansky]

You're still missing a space between while and the opening (.

Aargh! Fixed.

[quickfur]

Ah, finally style is passing...

[wilzbach]

Btw, what's the feature somebody mentioned where reviewers can edit the PR themselves? That would be very useful here instead of having so many roundtrips back and forth. How does that feature work?

http://wiki.dlang.org/Guidelines_for_maintainers#write-access-to-prs

[quickfur]

But publictests is still running into this snag:

out/std_regex_package.d(43): Error: undefined identifier 'popFrontN'
out/std_regex_package.d(150): Error: undefined identifier 'Yes.keepSeparators'
out/std_regex_package.d(154): Error: undefined identifier 'Yes.keepSeparators'

[wilzbach]

But publictests is still running into this snag:

This test is needed to ensure that all public unittests in dlang.org really compile and don't show import errors to the user.
FWIW the style Makefile target is also intended to be run locally - unfortunately only for Posix systems at the moment.

[quickfur]

Ah, so the problem is missing imports in a unittest block?

[wilzbach]

Ah, so the problem is missing imports in a unittest block?

Exactly - the script tries to simulate the execution on dlang.org by extracting all public unittests, putting them into a new file (one for each source file, slashes are replaced with underscores) and then subsequently trying to execute the extracted tests.

[quickfur]

Hmm, finally figured out how to push commits to somebody else's PR. That was rather non-obvious!

[quickfur]

Woohoo, publictests passed!

[quickfur]

Hmm, now it's complaining about code coverage. What's going on?

[quickfur]

OK, apparently too much of std.regex.internal.backtracking isn't covered by unittests.

Ping @DmitryOlshansky

(btw, I don't know how things are supposed to work after maintainers push commits to the PR; you may have to do a git pull to include the unittest fixes I pushed, otherwise circleCI may start complaining again.)

[wilzbach]

Hmm, now it's complaining about code coverage. What's going on?

At the moment I am just at my phone, so a couple of general general points:

• CircleCi runs sth. similar to make - f posix.mak std/string.test for all files (i.e. with -cov) and then uploads the results To CodeCov
• the test failures aren't required at the moment (so it's just a warning)
• the browser extension (https://github.com/codecov/browser-extension) saves a couple of clicks as it marks non-covered lines with red in the diff view
• code coverage doesn't with at CTFE

[dnadlinger]

OK, apparently too much of std.regex.internal.backtracking isn't covered by unittests.

It might just be that the code in question is CTFE-only (DMD doesn't currently track coverage data during CTFE execution, nor folds the data into the runtime stats). It's fine to ignore failures in that case; Codecov is not marked as "required" for this reason.

[DmitryOlshansky]

OK, apparently too much of std.regex.internal.backtracking isn't covered by unittests.

I don't buy it. The main reason is I think that coverage is computed by running the tests of the module in isolation. All tests of backtracking are hoisted out to tests.d so any addition of lines will negatively impact coverage in this model.

[wilzbach]

btw, I don't know how things are supposed to work after maintainers push commits to the PR; you may have to do a git pull

I am not sure about the process either, but it's essentially a force-push, so git pulling will set the local branch to the state of the remote branch.

[wilzbach]

I don't buy it. The main reason is I think that coverage is computed by running the tests of the module in isolation. All tests of backtracking are hoisted out to tests.d so any addition of lines will negatively impact coverage in this model.

FYI: The reason for this is that there was some weird, unexplainable behavior of the coverage feature that led to about 2K less covered lines when the test suite was ran with all tests in one go.
See: https://issues.dlang.org/show_bug.cgi?id=16397

[quickfur]

OK, in that case, I'm just gonna push the merge button. A bugfix for a serious bug as this one shouldn't be held up by these petty things.