Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Conversation

rainers
Copy link
Member

@rainers rainers commented Apr 12, 2015

This builds an appropriate TypeInfo_Struct for aaA.Entry at runtime whenever a new Impl is created.
I've removed the old unused _aaGetX as it doesn't provide enough type info, and disabled the future yet unused _aaGetZ for the same reason.

@rainers
Copy link
Member Author

rainers commented Apr 12, 2015

Failure caused by a bad test in the dmd test suite: dlang/dmd#4585

@MartinNowak
Copy link
Member

thx

@MartinNowak
Copy link
Member

Auto-merge toggled on

@MartinNowak MartinNowak added this to the 2.067.1 milestone Apr 12, 2015
@rainers
Copy link
Member Author

rainers commented Apr 12, 2015

I have no idea what's going on in the failing test, but here are some observations:

  • it passes on Win32, but also fails if std.regex.tests is built as a single test
  • the test is compiled on Win64, but never linked into the executable (unittest8.obj not listed in UNITTEST_OBJS)
  • The only AA with struct destructor in key or value I could find is Trie[CodepointSet], with CodepointSet = InversionList!GcPolicy, InversionList contains a CowArray!SP, and CowArray has a destructor.

Maybe CowArray does not expect to be called from the GC?

@rainers
Copy link
Member Author

rainers commented Apr 12, 2015

A few more observations:

  • the test passes if I comment the code in the destructor of std.uni.CowArray
  • reference counting in a destructor is dangerous, because the GC might call it from a different thread
  • the failing unittest is run single-threaded, so that's not the issue here
  • the dtor might change garbage collected in the same cycle, but that should only interfere with using debug=MEMSTOMP in the GC.

@schveiguy
Copy link
Member

I also noticed, the failing test passes in release mode, only fails in debug mode:

****** FAIL debug32 std.regex.internal.tests
...
0.464s PASS release32 std.regex.internal.tests

@rainers
Copy link
Member Author

rainers commented Apr 13, 2015

I also noticed, the failing test passes in release mode, only fails in debug mode:

That's probably because the failure is caused by an assert that is not compiled into the release build. I guess that's also why Win32 passes.

@MartinNowak
Copy link
Member

@rainers
Copy link
Member Author

rainers commented Apr 15, 2015

As far as I have investigated, the CodepointSet memoized in std.regex.internel.ir.wordCharacter() contains a released CowArray after a GC collection due to bad reference counting.

My current guess is this PR uncovers an unrelated bug with respect to postblits that didn't show up so far because the CowArrays just leaked.

@schveiguy
Copy link
Member

@rainers that makes a lot of sense.

@MartinNowak
Copy link
Member

Might also be https://issues.dlang.org/show_bug.cgi?id=14443.

@rainers
Copy link
Member Author

rainers commented Apr 17, 2015

I finally figured that there is a missing postblit when adding key values with this(this) to an AA.

I also added comments to aaKeys and aaValues about a possible fix for the missing postblits there, but it is blocked by these being pure nothrow.

MartinNowak added a commit that referenced this pull request Apr 17, 2015
fix Issue 14423 - struct destructors not finalized for AA values
@MartinNowak MartinNowak merged commit cfcf748 into dlang:master Apr 17, 2015
@MartinNowak MartinNowak modified the milestones: 2.068, 2.067.1 Apr 18, 2015
@MartinNowak
Copy link
Member

Let's rather not add it to the next point release though, as it might cause trouble for some people.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants