Fix issues 19655, 19656, 19657, 19746 & 19750 #9471

puneet · 2019-03-20T12:57:52Z

The Semantic (pass 1) analysis for classes is handled by visit(ClassDeclaration ) method of DsymbolSemanticVisitor class. For a given class, this method may be run multiple times in order to resolve forward references. The method incrementally tries to resolve the types referred to by the members of the class.

The subsequent calls to this method are short-circuited if the class members have been fully analyzed. For this the code tests that it is not the first/main call to the method (semanticRun == PASS.init else branch), scx is not set, and that the cldec.symtab is already set. If all these conditions are met, the method returns. But before returning, the method was setting cldec.semanticRun to PASS.semanticdone. It should not set semanticRun since the class has not been fully analyzed yet. The base class analysis for this class could be pending and as a result vtable may not have been fully created.

This fake setting of semanticRun results in the semantic analyzer to believe that the class has been fully analyzed. As exposed by the issues 19656, 19657, 19746 and 19750, it may result in compile time errors when a derived type class is getting analyzed and because of this fake semanticdone on the base class, the semantic analysis construes that an overridden method is not defined in the base class. Issue 19655 exposes a scenario where a buggy vtable may be created and a call to class method may result in execution of some adhoc code.

The Semantic (pass 1) analysis for classes is handled by visit(ClassDeclaration ) method of DsymbolSemanticVisitor class. For a given class, this method may be run multiple times in order to resolve forward references. The method incrementally tries to resolve the types referred to by the members of the class. The subsequent calls to this method are short-circuited if the class members have been fully analyzed. For this the code tests that it is not the first/main call to the method (semanticRun == PASS.init else branch), scx is not set, and that the cldec.symtab is already set. If all these conditions are met, the method returns. But before returning, the method was setting cldec.semanticRun to PASS.semanticdone. It should not set semanticRun since the class has not been fully analyzed yet. The base class analysis for this class could be pending and as a result vtable may not have been fully created. This fake setting of semanticRun results in the semantic analyzer to believe that the class has been fully analyzed. As exposed by the issues 19656, 19657, 19746 and 19750, it may result in compile time errors when a derived type class is getting analyzed and because of this fake semanticdone on the base class, the semantic analysis construes that an overridden method is not defined in the base class. Issue 19655 exposes a scenario where a buggy vtable may be created and a call to class method may result in execution of some adhoc code.

dlang-bot · 2019-03-20T12:57:55Z

Thanks for your pull request and interest in making D better, @puneet! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19655	regression	DMD generates wrong code for some circular dependencies
✓	19656	regression	D compiler fails to resolve circular module dependency when modules are compiled separately
✓	19657	regression	D compiler fails to resolve circular module dependency when modules are compiled together
✓	19746	regression	DMD fails to compile some circular dependencies spiced with is (T == super) condition
✓	19750	regression	[Reg v2.070.2] DMD fails with some circular module refs with Template/Mixin instances

Testing this PR locally

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

dub fetch digger
dub run digger -- build "stable + dmd#9471"

puneet · 2019-03-20T13:11:10Z

The tests fail because it is not able to create test_results/runnable/test19655.sh.out. The same happened with my local run. But a subsequent run was successful. Am I missing something?

thewilsonator · 2019-03-20T13:17:26Z

I think generally we have either shell script tests or source tests but not both. If the related files need to be compiled all together the they should go in extra_files. @ZombineDev @jacob-carlborg ?

UplinkCoder · 2019-03-20T13:32:52Z

@puneet can you consolidate your tests please?

puneet · 2019-03-20T14:32:40Z

@puneet can you consolidate your tests please?

I need multiple files because the import sequence is important to recreate the test scenario.

puneet · 2019-03-20T15:52:29Z

@puneet can you consolidate your tests please?

Alright, I get what you are saying. I now have created shell script wrappers that write out D files and then compiles. I will update the PR.

But I am still getting this error when I run the tests locally. But if I run make again, the error goes. As I understand the compile is happening too quickly and the makefile misses the output file. I can not make out why it happens for my sh files and not for the other sh tests that already exist.

Any clues?

Test compilable/test19656.sh failed. The logged output:
cat: test_results/compilable/test19656.sh.out: No such file or directory
Makefile:144: recipe for target 'test_results/compilable/test19656.sh.out' failed
make: *** [test_results/compilable/test19656.sh.out] Error 1

thewilsonator

The tests are a bit hacky, but then so it the whole test suite. Lets get this merged.

wilzbach · 2019-03-21T12:20:24Z

The tests are a bit hacky,

That doesn't even cut it. Let's please please please only use shell tests when absolutely necessary as they slow down the testsuite, are harder to debug and port to other runtimes.

Also, creating files in the shell scripts is an absolute anti-pattern.

@puneet thanks a lot for your PR, but could you please convert these test to proper test files?
It's not that hard:

you add your main entry point in either compilable or runnable
you add all imports into the respective imports directory (shell scripts typically use extra-files btw)
Add these files as EXTRA_SOURCES like this:

// EXTRA_SOURCES: imports/abc.d imports/abc2.d

wilzbach

Just a few comments regarding the tests. Can you please give someone who's experienced with the testsuite a reasonable timeframe for reviewing?

wilzbach · 2019-03-21T12:22:48Z

test/compilable/test19656.sh

+    void func (Thud ) { }
+    void thunk () { }
+}
+EOF


This should have been in extra-files instead of creating on-the-fly.

wilzbach · 2019-03-21T12:26:44Z

test/compilable/test19657.sh

@@ -0,0 +1,57 @@
+#! /usr/bin/env bash
+
+TEST_DIR=${OUTPUT_BASE}


TEST_DIR is an existing exported variable.

dmd/test/tools/exported_vars.sh

Line 3 in 4edae68

export TEST_DIR=$1 # TEST_DIR should be one of compilable, fail_compilation or runnable

While possible, probably not a good idea to overwrite it.

wilzbach · 2019-03-21T12:27:13Z

test/compilable/test19657.sh

+       ${TEST_DIR}${SEP}test19657e.d ${TEST_DIR}${SEP}test19657f.d \
+       ${TEST_DIR}${SEP}test19657g.d
+
+rm -f ${TEST_DIR}${SEP}${TEST_NAME}${OBJ} ${TEST_DIR}${SEP}${TEST_NAME}*.d


Wasn't the entire point of creating a directory that you can remove it in one command?

wilzbach · 2019-03-21T12:28:55Z

test/compilable/test19656.sh

+
+TEST_DIR=${OUTPUT_BASE}
+
+mkdir -p ${TEST_DIR}


Note that

(1) the files should have been not generated on the fly and placed e.g. in EXTRA_FILES

(2) ${OUTPUT_BASE}a would have worked too

dmd/test/tools/exported_vars.sh

Line 7 in 4edae68

export OUTPUT_BASE=${RESULTS_TEST_DIR}/${TEST_NAME} # reference to the resulting files without a suffix, e.g. test_results/runnable/test123

wilzbach · 2019-03-21T12:29:55Z

test/compilable/test19657.sh

+
+cat >${TEST_DIR}${SEP}test19657d.d <<EOF
+import test19657a;
+class Trump: Foo {}


Let's not be political here.

wilzbach · 2019-03-21T12:32:33Z

test/compilable/test19746.sh

@@ -0,0 +1,51 @@
+#! /usr/bin/env bash


This is an ill-formed pseudo she-bang header. It only works because the scripts aren't executed, but sourced.
I recommend removing it.

Hello @wilzbach, thanks for your feedback.

I am willing to implement your suggestions, but I see that the PR is already merged. Should I still go ahead push the modifications?

Yes, it was merged too eagerly. Please do so by opening a new PR. Thanks!

wilzbach · 2019-03-27T00:03:06Z

Ping. @thewilsonator - this is yet another example why waiting for more than a day is very important. Follow-ups to a PR happen rarely and thus the debt just aggregates ...

MoonlightSentinel · 2020-03-13T13:55:31Z

Rework as non-shell test: #10908

Fix issues 19655, 19656, 19657, 19746 & 19750 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

[dmd-cxx] Fix issues 19655, 19656, 19657, 19746 & 19750 (#9471)

dlang-bot added the Bug Fix label Mar 20, 2019

puneet mentioned this pull request Mar 20, 2019

Fix for issue 19656 #9462

Closed

puneet added 3 commits March 20, 2019 21:53

Consolidated tests into shell scripts

aea418a

Make tests compatible with Windows Environment

ee6e513

Added -m${MODEL} flag to all DMD invocations for tests

a7491ab

thewilsonator approved these changes Mar 21, 2019

View reviewed changes

thewilsonator added the auto-merge-squash label Mar 21, 2019

dlang-bot merged commit 58878ae into dlang:stable Mar 21, 2019

wilzbach reviewed Mar 21, 2019

View reviewed changes

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Mar 13, 2020

Fix issues 19655, 19656, 19657, 19746 & 19750 (dlang#9471)

968386f

Fix issues 19655, 19656, 19657, 19746 & 19750 merged-on-behalf-of: Nicholas Wilson <thewilsonator@users.noreply.github.com>

ibuclaw added a commit that referenced this pull request Mar 13, 2020

Merge pull request #10913 from ibuclaw/dmd-cxx-2085fixes

420b220

[dmd-cxx] Fix issues 19655, 19656, 19657, 19746 & 19750 (#9471)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues 19655, 19656, 19657, 19746 & 19750 #9471

Fix issues 19655, 19656, 19657, 19746 & 19750 #9471

puneet commented Mar 20, 2019

dlang-bot commented Mar 20, 2019

puneet commented Mar 20, 2019

thewilsonator commented Mar 20, 2019

UplinkCoder commented Mar 20, 2019

puneet commented Mar 20, 2019

puneet commented Mar 20, 2019

thewilsonator left a comment

wilzbach commented Mar 21, 2019

wilzbach left a comment

wilzbach Mar 21, 2019

wilzbach Mar 21, 2019

wilzbach Mar 21, 2019

wilzbach Mar 21, 2019

wilzbach Mar 21, 2019

wilzbach Mar 21, 2019

puneet Mar 21, 2019

wilzbach Mar 21, 2019

wilzbach commented Mar 27, 2019

MoonlightSentinel commented Mar 13, 2020


		TEST_DIR=${OUTPUT_BASE}

		mkdir -p ${TEST_DIR}

Fix issues 19655, 19656, 19657, 19746 & 19750 #9471

Fix issues 19655, 19656, 19657, 19746 & 19750 #9471

Conversation

puneet commented Mar 20, 2019

dlang-bot commented Mar 20, 2019

Bugzilla references

Testing this PR locally

puneet commented Mar 20, 2019

thewilsonator commented Mar 20, 2019

UplinkCoder commented Mar 20, 2019

puneet commented Mar 20, 2019

puneet commented Mar 20, 2019

thewilsonator left a comment

Choose a reason for hiding this comment

wilzbach commented Mar 21, 2019

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Mar 27, 2019

MoonlightSentinel commented Mar 13, 2020