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

DCD stops working if some thread is created after instantiation of ModuleCache #407

Closed
buggins opened this issue Aug 11, 2017 · 26 comments
Closed
Labels

Comments

@buggins
Copy link

buggins commented Aug 11, 2017

I'm using DCD as a library in DlangIDE.
All DCD calls are made from separate thread.
DCD works ok until some thread is created (e.g. to invoke DUB for building).
After this operation DCD stops working correctly and can locate symbols only from current source file.

Root cause:
If some another thread is created after DCD thread instantiated ModuleCache, DCD is not able to process goToDefinition for symbol and other functions if they are related to other modules than current source file.
Probably, it's related to ModuleCache and/or allocators.

Steps to reproduce:
After creation of ModuleCache, start some thread and wait until it's finished.

/// call this function after DCD ModuleCache is instantiated to reproduce issue
void testDCDFailAfterThreadCreation() {
    import core.thread;
    Thread thread = new Thread(delegate() {
        Thread.sleep(dur!"msecs"(2000));
    });
    thread.start();
    thread.join();
}
@buggins
Copy link
Author

buggins commented Aug 14, 2017

autocomplete.d
getSymbolsByTokenChain()
symbols = completionScope.getSymbolsByNameAndCursor(stringToken(tokens[0]), cursorPosition);
returns symbol[0].type is null after new thread is created

@ghost ghost added the bug label Aug 14, 2017
@ghost
Copy link

ghost commented Aug 14, 2017

It doesn't look like a trivial bug. Can you provide a small DUB project that reproduces it simply by running dub run ?

@buggins
Copy link
Author

buggins commented Aug 14, 2017

Ok. I need some time to do it.

@ghost
Copy link

ghost commented Aug 14, 2017

Or instead of a project a single module with a DUB SDL header. This system allows to put dependencies too.

@buggins
Copy link
Author

buggins commented Aug 14, 2017

Created sample project to reproduce issue: https://github.com/buggins/dcdtest

Test results:

Performing "debug" build using dmd for x86.
emsi_containers 0.5.3: target for configuration "library" is up to date.
libdparse 0.7.1-beta.9: target for configuration "library" is up to date.
dsymbol 0.2.7: target for configuration "library" is up to date.
msgpack-d 1.0.0-beta.6: target for configuration "library" is up to date.
dcd 0.9.0: target for configuration "library" is up to date.
dcdtest ~master: building configuration "application"...
Linking...
To force a rebuild of up-to-date targets, run again with --force.
Running .\bin\dcdtest.exe
test dir: D:\projects\d\dcdtest\test
source file: D:\projects\d\dcdtest\test\main.d
source: import testmodule;

int main(string[] args) {

Test.run();

}

position: 59
Calling findDeclaration()
result file: D:\projects\d\dcdtest\test\testmodule.d location: 80
New thread created
Calling findDeclaration()
result file: location: 0

@buggins
Copy link
Author

buggins commented Aug 14, 2017

Test:
invoke findDeclaration() - found
create some thread, start it, then wait until thread is exited
invoke findDeclaration() - not found

dmd --version
DMD32 D Compiler v2.075.0
Copyright (c) 1999-2017 by Digital Mars written by Walter Bright

dub --version
DUB version 1.0.0, built on Jun 20 2016

Windows 7

@ghost
Copy link

ghost commented Aug 14, 2017

Thanks, i can see the problem on linux x86-64 too.

@ghost
Copy link

ghost commented Aug 14, 2017

After a GDB session, i've found something interesting in dsymbol.scope_.d, line 131

	inout(DSymbol)*[] getSymbolsByName(istring name) inout
	{
		import std.array : array, appender;
		import std.algorithm.iteration : map;

		DSymbol s = DSymbol(name);
		auto er = _symbols.equalRange(SymbolOwnership(&s));
		if (!er.empty)
			return cast(typeof(return)) array(er.map!(a => a.ptr));
  • after the thread created, er is always empty, not before.
  • name parameter have always the same value, before and after the thread.
  • I couldn't see s value (print s always the same grarbages)

So far it looks like it's auto er = _symbols.equalRange(SymbolOwnership(&s)); that behaves differently.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Thanks! Will try to investigate it further.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

I see strange behavior.
Added debug dump of name parameter in getSymbolsByName - string and its address.
Strings should be interned, and have the same pointer value.
Pointers are different for "int" and "long" before and after creation of new thread:

2017-08-15T09:53:45.056:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName long ptr=650B6EA
2017-08-15T09:53:45.056:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=650B6C0

2017-08-15T09:53:45.056:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName string ptr=650C8DE
2017-08-15T09:53:45.056:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName X ptr=671D177
2017-08-15T09:53:45.057:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=650B6C0
2017-08-15T09:53:45.057:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=650B6C0
2017-08-15T09:53:45.057:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName Window ptr=C91F5A5
2017-08-15T09:53:45.057:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName window ptr=65AC608

// new thread is created

2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName long ptr=10EBA0EA
2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=10EBA0C0

2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName string ptr=650C8DE
2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName X ptr=671D177
2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=10EBA0C0
2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName int ptr=10EBA0C0

2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName Window ptr=C91F5A5
2017-08-15T09:54:57.462:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByNameAndCursor:193 calling getSymbolsByName window ptr=65AC608

Cannot reproduce it by adding test string interning in sample app.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Added check for improperly interned strings to getSymbolsByName:

inout(DSymbol)*[] getSymbolsByName(istring name) inout
{
    import dsymbol.string_interning;
    import dsymbol.symbol;
    istring interned = internString(name);
    //warning("calling getSymbolsByName ", name, " ptr=", name.ptr);
    if (interned.ptr != name.ptr) {
        warning("dsymbol.Scope getSymbolsByName: string `", name, "` is not interned! Proper interned ptr=", interned.ptr, " Real ptr=", name.ptr);
    }

Before thread creation there are no warnings.

After thread creation there are some non-interned strings detected:

2017-08-15T10:37:38.818:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `long` is not interned! Proper interned ptr=64CB722 Real ptr=D0314DA
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0
2017-08-15T10:38:06.370:deps\DCD\dsymbol\src\dsymbol\scope_.d:getSymbolsByName:138 dsymbol.Scope getSymbolsByName: string `int` is not interned! Proper interned ptr=64CB6F8 Real ptr=D0314B0

stack traces look like

0x00508E81 in dsymbol.scope_.Scope.getSymbolsByNameinout(inout(dsymbol.symbol.DSymbol)*[] function(dsymbol.string_interning.InternedString)) at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\scope_.d(141)
0x00509528 in dsymbol.symbol.DSymbol*[] dsymbol.scope_.Scope.getSymbolsByNameAndCursor(dsymbol.string_interning.InternedString, uint) at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\scope_.d(209)
0x0050A132 in D7dsymbol10conversion6second19resolveTypeFromTypeFPS7dsymbol6symbol7DSymbolPS7dsymbol11type_loo8E3C93C613D5C0EC2290333612E9561C at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\conversion\second.d(240)
0x0050AABA in D7dsymbol10conversion6second11resolveTypeFPS7dsymbol6symbol7DSymbolKS10containers12unrolledlist15003144AE64E08EE817E65F878EC7DE at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\conversion\second.d(422)
0x00509A88 in void dsymbol.conversion.second.secondPass(dsymbol.semantic.SemanticSymbol*, dsymbol.scope_.Scope*, ref dsymbol.modulecache.ModuleCache) at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\conversion\second.d(58)
0x00509AE5 in void dsymbol.conversion.second.secondPass(dsymbol.semantic.SemanticSymbol*, dsymbol.scope_.Scope*, ref dsymbol.modulecache.ModuleCache) at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\conversion\second.d(81)
0x0050828C in D7dsymbol10conversion25generateAutocompleteTreesFAxS3std12experimental5lexer667__T14TokenStruct828CF9EF48D954C0C0558CA8F1DEAF24 at D:\projects\d\dlangide\deps\DCD\dsymbol\src\dsymbol\conversion\package.d(51)
0x0044EEE7 in D6server12autocomplete23getSymbolsForCompletionFxS6common8messages19AutocompleteRequestxE6commo94F46887A108B4B36C806928512CD087 at D:\projects\d\dlangide\deps\DCD\src\server\autocomplete.d(483)
0x0044E2CC in common.messages.AutocompleteResponse server.autocomplete.findDeclaration(const(common.messages.AutocompleteRequest), ref dsymbol.modulecache.ModuleCache) at D:\projects\d\dlangide\deps\DCD\src\server\autocomplete.d(221)

Tried to modify source file for which symbol lookup is requested.

I see warnings for built in types: long, ulong, float, double, int...

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Update:

Issue is probably caused by dsymbol.builtin.names constants.
They are not thread local.

I've modified example to dump builtinTypeNames[0..5] before, inside and after new thread.

Performing "debug" build using dmd for x86.
emsi_containers 0.5.3: target for configuration "library" is up to date.
libdparse 0.7.1-beta.9: target for configuration "library" is up to date.
dsymbol 0.2.7: target for configuration "library" is up to date.
msgpack-d 1.0.0-beta.6: target for configuration "library" is up to date.
dcd 0.9.0: target for configuration "library" is up to date.
dcdtest ~master: building configuration "application"...
Linking...
To force a rebuild of up-to-date targets, run again with --force.
Running .\bin\dcdtest.exe 
Dumping interned strings before thread start
Interned strings ptr=57D550
name=int ptr=EB1AD0 interned.ptr=EB1AD0
name=uint ptr=EB1AD3 interned.ptr=EB1AD3
name=double ptr=EB1AD7 interned.ptr=EB1AD7
name=idouble ptr=EB1ADD interned.ptr=EB1ADD
name=float ptr=EB1AE4 interned.ptr=EB1AE4
inside thread
Interned strings ptr=57D550
name=int ptr=EC5430 interned.ptr=EC5430
name=uint ptr=EC5433 interned.ptr=EC5433
name=double ptr=EC5437 interned.ptr=EC5437
name=idouble ptr=EC543D interned.ptr=EC543D
name=float ptr=EC5444 interned.ptr=EC5444
exiting thread
New thread created
Dumping interned strings after thread exit
Interned strings ptr=57D550
name=int ptr=EC5430 interned.ptr=EB1AD0
name=uint ptr=EC5433 interned.ptr=EB1AD3
name=double ptr=EC5437 interned.ptr=EB1AD7
name=idouble ptr=EC543D interned.ptr=EB1ADD
name=float ptr=EC5444 interned.ptr=EB1AE4

When new thread is created, dsymbol.builtin.names constants will be changed to allocated in new thread string cache.

Updated code:

https://github.com/buggins/dcdtest

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Following hack does not work if some thread is already created before DCD thread.

dsymbol.builtin.names:

static this()
{
    if (builtinTypeNames[0].data.ptr)
        return;

Removing of immutable breaks pure declarations.

Another dirty hack works ok:

private __gshared bool BUILTIN_NAMES_LOCKED = false;
void lockBuiltinNames() {
    BUILTIN_NAMES_LOCKED = true;
}
/**
 * Initializes builtin types and the various properties of builtin types
 */
static this()
{
    if (BUILTIN_NAMES_LOCKED)
        return;

lockBuiltinNames() has to be called from thread which will be used for DCD.

@ghost
Copy link

ghost commented Aug 15, 2017

Yes you've found it. without the immutable + without the pure in second.d it works.

@ghost
Copy link

ghost commented Aug 15, 2017

BTW i've tested without the hack, here i still have the first version of the test case. It means that just a PR in DSymbol can fix the issue.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

If removing of immutable is ok, I could create PR.

@ghost
Copy link

ghost commented Aug 15, 2017

In names.d replace immutable by const and also remove the pure here.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Will const be thread locals?

@ghost
Copy link

ghost commented Aug 15, 2017

no, sorry it was a mistake to sat that. don't put const.

@ghost
Copy link

ghost commented Aug 15, 2017

By the way i've verified and the changes don't break DCD so you can really make the PR now.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Created PR.
How long it would take to get new Dsymbol and DCD releases?

@ghost
Copy link

ghost commented Aug 15, 2017

not much. 1 hour max because the bot at code.dlang.org has to detect the tag.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Are you going to release new label of DCD with updated DSymbol?

@ghost
Copy link

ghost commented Aug 15, 2017

do you need one ? I can certainly push an alpha label. By the way i need review + approval for the DCD PR.

@buggins
Copy link
Author

buggins commented Aug 15, 2017

Trying to resolve it by adding DlangIDE dependency
"dsymbol": "~>0.2.8",

Seems working for me.
So I can live w/o new DCD label.

Thank you a lot for your help!

@ghost ghost closed this as completed in bd32bc9 Aug 15, 2017
ghost pushed a commit that referenced this issue Aug 15, 2017
fix #407 - DCD stops working if some thread is created after instanti…
@ghost
Copy link

ghost commented Aug 15, 2017

I've set 0.9.1 because there was a few things anyway since 0.9.0

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant