Fix issue 16995 - __traits(getUnitTests) works with separate compilation by atilaneves · Pull Request #6727 · dlang/dmd

atilaneves · 2017-04-24T23:29:40Z

Before this patch, __traits(getUnitTests) only worked when compiling all source files at once. This fix enables it to work for separate compilation as well.

UplinkCoder · 2017-06-06T18:09:34Z

src/ddmd/func.d

 extern (C++) static Identifier unitTestId(Loc loc)
 {
+    // there's a counter per module in case there are two unit tests on one line
+    static __gshared int[const(char)*] counterPerModule;


I don't want AA's in DMD :)
That we use them in dtemplate is a mistake.

You are quite far from the point where "I don't want AA's [sic] in DMD" is enough of a justification – it wouldn't even be for Walter. ;) Why should we maintain a separate (mostly worse) AA implementation in addition to the druntime one?

Also, please consider helping the more inexperienced DMD contributors by suggesting solutions rather than just saying "this is wrong". In this particular case, it seems like the best solution would be to delay setting the actual function name until semantic(), where a unique string can be generated from the Scope (similar to how things are done for lambdas, or just by looking up the Module and keeping a counter there).

(I presume the problems comes with template instantiations; if two test on one line were the issue as the comment suggests, the easiest solution would just be to add the column number as well.)

@klickverbot

You are quite far from the point where "I don't want AA's [sic] in DMD" is enough of a justification – it wouldn't even be for Walter. ;)

I am not sure if this was meant jokingly or condescending ... that aside.

relying on d-runtime is not great because then the performance of the compiler is determined be the host druntime...

though I guess for a feature such as this it should not have too much of a impact.

I am not sure if this was meant jokingly or condescending

At least as jokingly as your "I don't want […]" must have been, of course. ;)

relying on d-runtime is not great because then the performance of the compiler is determined be the host druntime...

It already is determined by the host compiler to great extent (compare a LDC LTO build against a DMD-compiled one if you're curious). But if you are worried, you can always do a multi-stage build and have the compiler self-host.

I very much doubt that any discernible difference will exist in performance due to naming unittest blocks. Delaying where the function name is set seems to be quite involved work - I'm not sure it's worth it. Especially since we're talking about how unittests are named here.

Of course, if needs be...

@klickverbot Perhaps, but that's well beyond the scope of this PR - it's an overhaul, however minor, of the compiler. Also, __traits(getUnitTests) is a bit... special. It seems unlikely (to me anyway) that the "resetability" will affect any other features in a similar way. Lambdas aren't referenced by name by other modules.

Lambdas aren't referenced by name by other modules.

They are quite frequently when using templates. This is why the problem needed to be solved properly for them already.

@klickverbot In a way that causes linker errors? I just tried a 1000-line lambda (I assume this means it won't be inlined) across modules and separate compilation was fine.

If this counter issue manifests in other ways when compiling separately, then I'm all for doing it right: it's pointless to whack one mole and leave the others. However, unless there's a clear bug elsewhere and not just with __traits(getUnitTests) then I think it's a big ask for me to learn the compiler internals to refactor it. Would it be nicer another way? Probably, but that's a different issue. If there are known bugs relating to this please let me know.

This patch fixes the bug, allows unittest builds with __traits(getUnitTests) to be built by package as is recommended and IMHO isn't entirely horrible.

In a way that causes linker errors? I just tried a 1000-line lambda (I assume this means it won't be inlined) across modules and separate compilation was fine.

Yes, it doesn't cause any linker errors precisely because lambdas are already implemented properly.

If this counter issue manifests in other ways when compiling separately, then I'm all for doing it right: it's pointless to whack one mole and leave the others.

It manifests itself in templated unit tests:

module foo; struct Foo(T) { unittest {} }

module bar1; import foo; pragma(msg, "bar1/int: ", __traits(getUnitTests, Foo!int));

module bar2; import foo; pragma(msg, "bar2/float: ", __traits(getUnitTests, Foo!float)); pragma(msg, "bar2/int: ", __traits(getUnitTests, Foo!int));

bar1/int: tuple(__unittest_foo_d_2_1)

bar2/float: tuple(__unittest_foo_d_2_1) bar2/int: tuple(__unittest_foo_d_2_2)

(Had I realized this sooner, I would have pointed it out before, of course.)

@klickverbot That's a user case which is highly unlikely to ever happen in practice, but I see your point.

wilzbach · 2017-06-07T03:55:15Z

@atilaneves: If you exclude the # from the commit message, it will auto-magically work the Dlang-Bot (the easiest way is just to copy/paste the Bugzilla title and prepend "Fix " to it).

For more infos:

https://github.com/dlang-bots/dlang-bot#automated-references

wilzbach

I guess you want to remove foo.txt or is this for testing/triggering a CI?

dlang-bot · 2017-06-07T13:07:09Z

Thanks for your pull request, @atilaneves! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	16995	__traits(getUnittests) doesn't work with separate compilation

atilaneves · 2017-06-07T13:07:46Z

@wilzbach Yes, forgot to delete foo.txt. I changed the commit message as suggested.

atilaneves · 2017-07-19T19:33:51Z

@klickverbot @UplinkCoder I changed the PR as requested - the identifier is now generated at semantic, similarly to what is done for lambdas. There is no longer an associative array or dependency on druntime.

UplinkCoder · 2017-07-19T20:36:17Z

@atilaneves the .gitignore changes are unrelated, please pull them out.
Otherwise, Great work!

PetarKirov · 2017-09-22T13:06:58Z

@wilzbach @ibuclaw I think all of your concerns were addressed. Does this now look good to you?

Thanks for pushing this.

ibuclaw · 2017-09-22T15:51:00Z

LGTM.

PetarKirov · 2017-09-22T17:14:50Z

(@ibuclaw you need to mark this PR as approved, as now it reads "ibuclaw requested changes")

ibuclaw · 2017-09-22T18:05:32Z

@ZombineDev - you had your own nit. ☺️

PetarKirov · 2017-09-22T20:09:01Z

Yes, my comment still stands, I was just trying to "clean-up" the pull request so that it's more clear what's left to be done.

atilaneves · 2017-09-24T14:18:39Z

@ZombineDev I added static as requested and as should have been in the 1st place.

PetarKirov · 2017-09-24T20:40:29Z

@ibuclaw this should be now good to go.

…ompilation

WalterBright · 2017-09-29T21:58:25Z

src/ddmd/dsymbolsem.d

    {
+        // the identifier has to be generated here in order to be able to link
+        // or not the files are compiled separately or all at once.
+        // See bugzilla #16995


Use actual link:

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

WalterBright · 2017-09-29T21:59:08Z

src/ddmd/dsymbolsem.d

    override void visit(UnitTestDeclaration utd)
    {
+        // the identifier has to be generated here in order to be able to link
+        // or not the files are compiled separately or all at once.


I can guess what the sentence means, but it is just a guess. Please fix grammar!

I think that was a rebase issue. Fixing it all the same.

WalterBright · 2017-09-29T21:59:45Z

src/ddmd/dsymbolsem.d

+        // the identifier has to be generated here in order to be able to link
+        // or not the files are compiled separately or all at once.
+        // See bugzilla #16995
+        utd.setIdentifier;


Being a function, please call it with utd.setIdentifier();

WalterBright · 2017-09-29T22:01:51Z

src/ddmd/func.d


+    final void setIdentifier()
+    {
+        ident = createIdentifier(loc, _scope);


Does calling this replace the temporary one above? Is it always called? Need Ddoc comment for setIdentifier.

It does. I added ddoc to explain that.

WalterBright · 2017-09-29T22:02:52Z

src/ddmd/func.d

-        super(loc, endloc, unitTestId(loc), stc, null);
+        // Id.empty can cause certain things to fail, so we create a
+        // temporary one here that serves for most purposes with
+        // createIdentifier. There is no scope to pass so we pass null.


It makes me uneasy to start with one identifier, and finish with another. What fails with an empty identifier?

I didn't like having to do it either, my first attempt was to use Id.empty, but when I did that the unit tests failed. I can't remember which ones or why now, but they do.

WalterBright · 2017-09-29T22:06:12Z

src/ddmd/dmodule.d

    bool isPackageFile;         // if it is a package.d
    int needmoduleinfo;
-
+    uint unitTestCounter;       // how many unittests have been seen so far


Document that unit test counter is per module, rather than global, so that it is reproducible regardless of how it is compiled.

WalterBright · 2017-09-29T22:08:41Z

src/ddmd/func.d

+        // replace characters that demangle can't handle
+        auto str = buf.peekString;
+        for(int i = 0; str[i] != 0; ++i)
+            if(str[i] == '/' || str[i] == '\\' || str[i] == '.') str[i] = '_';


More efficient as:

foreach (ref c; buf.peekSlice()) { if (c == '/' || c == ...) c = '_'; }

Or even: buf.peekSlice()[11 .. $-4] :-)

peekSlice returns const(char)[] so that doesn't compile. The latter version is also too hardcoded, even though I think it's unlikely anybody will change the way unit tests are named in the future.

here's one of those places where among is useful (though we can't use it here)

so that doesn't compile

Oopsie-daisy!

This test is not strong enough and various testsuite tests fail if compiling to assembly (such as what gdc does).

This logic should match what is done for pragma(mangle).

dmd/src/ddmd/dsymbolsem.d

Lines 3033 to 3045 in 32830de

if (c < 0x80)

{

if (c >= 'A' && c <= 'Z' || c >= 'a' && c <= 'z' || c >= '0' && c <= '9' || c != 0 && strchr("$%().:?@[]_", c))

{

++i;

continue;

}

else

{

pd.error("char 0x%02x not allowed in mangled name", c);

break;

}

}

For instance, unittests compiled that are inside the directory extra-files fail because the assembler stumbles over on the dash symbol.

WalterBright · 2017-09-29T22:11:51Z

Am I correct in surmising that this change just replaces a global counter with a per-module one?

atilaneves · 2017-09-30T10:12:08Z

@WalterBright Yes, the change replaces a global counter with a per-module one.

andralex

I'm good with this, imperfect but leaves a bugzilla for a related improvement. @WalterBright?

andralex · 2017-09-30T16:41:10Z

src/ddmd/func.d

+    {
+        OutBuffer buf;
+        auto index = sc ? sc._module.unitTestCounter++ : 0;
+        buf.printf("__unittest_%s_%u_%d", loc.filename, loc.linnum, index);


There's a difference without a distinction between %u and %d - just use %u for both, or better yet %s for all (assuming they're all unsigned as they should).

printf doesn't work with %s for integers :-)

Oh wait I thought it's writef...

andralex · 2017-09-30T16:42:25Z

src/ddmd/func.d

+        // replace characters that demangle can't handle
+        auto str = buf.peekString;
+        for(int i = 0; str[i] != 0; ++i)
+            if(str[i] == '/' || str[i] == '\\' || str[i] == '.') str[i] = '_';


here's one of those places where among is useful (though we can't use it here)

JohanEngelen · 2017-12-17T20:08:15Z

@atilaneves Could you take a look at this bug report? https://issues.dlang.org/show_bug.cgi?id=18097 Much obliged.

atilaneves · 2017-12-18T10:56:20Z

@JohanEngelen I looked at it. I'd have to debug to figure it out. There's another problem with my fix (I haven't filed the bug yet) so I'll talke a look at it when I do that.

atilaneves force-pushed the fix_get_unittests_bug branch 2 times, most recently from 6d072cc to b868e89 Compare May 10, 2017 13:23

atilaneves force-pushed the fix_get_unittests_bug branch 7 times, most recently from c6e5691 to b426b7b Compare May 25, 2017 23:19

atilaneves force-pushed the fix_get_unittests_bug branch 2 times, most recently from f31977e to 5b92947 Compare June 2, 2017 10:49

atilaneves force-pushed the fix_get_unittests_bug branch 6 times, most recently from d7cac27 to 8d7d387 Compare June 6, 2017 17:41

UplinkCoder reviewed Jun 6, 2017

View reviewed changes

wilzbach previously requested changes Jun 7, 2017

View reviewed changes

atilaneves force-pushed the fix_get_unittests_bug branch from 4748c5b to f6818da Compare June 7, 2017 13:07

dlang-bot added the Severity:Bug Fix label Jun 7, 2017

atilaneves force-pushed the fix_get_unittests_bug branch from f6818da to caff09a Compare July 18, 2017 19:18

dlang-bot added the Severity:Enhancement label Jul 18, 2017

atilaneves force-pushed the fix_get_unittests_bug branch from caff09a to ea52245 Compare July 18, 2017 20:55

atilaneves force-pushed the fix_get_unittests_bug branch from e53b44a to b918e1d Compare July 19, 2017 20:49

ibuclaw approved these changes Sep 22, 2017

View reviewed changes

atilaneves force-pushed the fix_get_unittests_bug branch from 5c106d2 to c4b113f Compare September 24, 2017 14:17

atilaneves force-pushed the fix_get_unittests_bug branch from c4b113f to c405271 Compare September 24, 2017 14:38

Fix Issue 16995 - __traits(getUnittests) doesn't work with separate c…

f527810

…ompilation

atilaneves force-pushed the fix_get_unittests_bug branch from c405271 to f527810 Compare September 29, 2017 15:50

WalterBright reviewed Sep 29, 2017

View reviewed changes

Address review comments

0f26862

andralex approved these changes Sep 30, 2017

View reviewed changes

WalterBright merged commit ebd6606 into dlang:master Sep 30, 2017

atilaneves deleted the fix_get_unittests_bug branch October 10, 2017 16:26

wilzbach mentioned this pull request Dec 26, 2017

Run CircleCi with BUILD=debug #7528

Merged

jacob-carlborg mentioned this pull request Dec 27, 2017

Fix invalid characters in mangling #7531

Merged

	if (c < 0x80)
	{
	if (c >= 'A' && c <= 'Z' \|\| c >= 'a' && c <= 'z' \|\| c >= '0' && c <= '9' \|\| c != 0 && strchr("$%().:?@[]_", c))
	{
	++i;
	continue;
	}
	else
	{
	pd.error("char 0x%02x not allowed in mangled name", c);
	break;
	}
	}

Uh oh!

Conversation

atilaneves commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jun 7, 2017

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

atilaneves commented Jun 7, 2017

Uh oh!

atilaneves commented Jul 19, 2017

Uh oh!

UplinkCoder commented Jul 19, 2017

Uh oh!

PetarKirov commented Sep 22, 2017

Uh oh!

ibuclaw commented Sep 22, 2017

Uh oh!

PetarKirov commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibuclaw commented Sep 22, 2017

Uh oh!

PetarKirov commented Sep 22, 2017

Uh oh!

atilaneves commented Sep 24, 2017

Uh oh!

PetarKirov commented Sep 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

atilaneves commented Apr 24, 2017 •

edited

Loading

dnadlinger Jun 7, 2017 •

edited

Loading

dlang-bot commented Jun 7, 2017 •

edited

Loading

PetarKirov commented Sep 22, 2017 •

edited

Loading

WalterBright Sep 29, 2017 •

edited

Loading