bdw-gc 7.4.0 disclaim_test

passes on Solaris  8 SPARC 32bit
fails  on Solaris  8 SPARC 64bit
passes on Solaris  9 SPARC 32bit
fails  on Solaris  9 SPARC 64bit
passes on Solaris 10 SPARC 32bit
fails  on Solaris 10 SPARC 64bit
fails  on Solaris 11 SPARC 64bit
passes on Solaris 11 i86pc 32bit
passes on Solaris 11 i86pc 32bit
passes on Solaris 11 i86pc 64bit

So, 64 bit SPARC is an issue. This could be related to this stack size discussion:
http://lists.ximian.com/pipermail/mono-list/2007-March/034535.html

Digging into it more.

from bdwgc.

Redirecting to ML

Tue, 13 May 2014, 09:07 +04:00 from marksolaris [email protected]:
bdw-gc 7.4.0 disclaim_test
passes on Solaris  8 SPARC 32bit
fails  on Solaris  8 SPARC 64bit
passes on Solaris  9 SPARC 32bit
fails  on Solaris  9 SPARC 64bit
passes on Solaris 10 SPARC 32bit
fails  on Solaris 10 SPARC 64bit
fails  on Solaris 11 SPARC 64bit
passes on Solaris 11 i86pc 32bit
passes on Solaris 11 i86pc 32bit
passes on Solaris 11 i86pc 64bit
So, 64 bit SPARC is an issue. This could be related to this stack size discussion:
http://lists.ximian.com/pipermail/mono-list/2007-March/034535.html
Digging into it more.
—

from bdwgc.

Hi Petter,

marksolaris supplied details about threaded disclaim_test failure on Solaris Sparc 64 ( #42 ).
Could you please have a look at? Thank you

Mon, 12 May 2014 00:05:14 -0700 from marksolaris [email protected]:

(I commented out the test_cpp test to move forward, it's still broken)
host:/var/tmp/bdw-gc-7.4.0 root# gmake check
gmake[1]: Entering directory /var/tmp/bdw-gc-7.4.0' /usr/local/bin/gmake libstaticrootslib_test.la libstaticrootslib2_test.la cordtest gctest leaktest middletest smashtest hugetest realloc_test staticrootstest threadleaktest threadkey_test subthreadcreate_test initsecondarythread_test disclaim_test disclaim_bench gmake[2]: Entering directory/var/tmp/bdw-gc-7.4.0'
gmake[2]: libstaticrootslib_test.la' is up to date. gmake[2]:libstaticrootslib2_test.la' is up to date.
gmake[2]: cordtest' is up to date. gmake[2]:gctest' is up to date.
gmake[2]: leaktest' is up to date. gmake[2]:middletest' is up to date.
gmake[2]: smashtest' is up to date. gmake[2]:hugetest' is up to date.
gmake[2]: realloc_test' is up to date. gmake[2]:staticrootstest' is up to date.
gmake[2]: threadleaktest' is up to date. gmake[2]:threadkey_test' is up to date.
gmake[2]: subthreadcreate_test' is up to date. gmake[2]:initsecondarythread_test' is up to date.
gmake[2]: disclaim_test' is up to date. gmake[2]:disclaim_bench' is up to date.
gmake[2]: Leaving directory /var/tmp/bdw-gc-7.4.0' /usr/local/bin/gmake check-TESTS gmake[2]: Entering directory/var/tmp/bdw-gc-7.4.0'
gmake[3]: Entering directory /var/tmp/bdw-gc-7.4.0' PASS: cordtest PASS: gctest PASS: leaktest PASS: middletest PASS: smashtest PASS: hugetest PASS: realloc_test PASS: staticrootstest PASS: threadleaktest PASS: threadkey_test PASS: subthreadcreate_test PASS: initsecondarythread_test FAIL: disclaim_test PASS: disclaim_bench gmake[4]: Entering directory/var/tmp/bdw-gc-7.4.0'
gmake[5]: Entering directory /var/tmp/bdw-gc-7.4.0' gmake[5]: Nothing to be done forall-am'.
gmake[5]: Leaving directory `/var/tmp/bdw-gc-7.4.0'

gmake[4]: Leaving directory `/var/tmp/bdw-gc-7.4.0'

Testsuite summary for gc 7.4.0

TOTAL: 14

PASS: 13

SKIP: 0

XFAIL: 0

FAIL: 1

XPASS: 0

ERROR: 0

See ./test-suite.log

Please report to [email protected]

gmake[3]: *** [test-suite.log] Error 1
gmake[3]: Leaving directory `/var/tmp/bdw-gc-7.4.0'
gmake[2]: *** [check-TESTS] Error 2
host:/var/tmp/bdw-gc-7.4.0 root# cat disclaim_test.log
Assertion failure, line 102: p->checksum == checksum
Threaded disclaim test.

DEBUG: Destruct 100401000 = (1003fa640, 0)
DEBUG: checksum 86802:
DEBUG: p->checksum 86802:

DEBUG: Destruct 100401020 = (0, 0)
DEBUG: checksum 782:
DEBUG: p->checksum 782:

DEBUG: Destruct 100401080 = (0, 0)
DEBUG: checksum 782:
DEBUG: p->checksum 782:

DEBUG: Destruct 1004010a0 = (100400f60, 1003f59a0)
DEBUG: checksum 4206810:
DEBUG: p->checksum 10948:

Problem section (and debugging mods)
host:/var/tmp/bdw-gc-7.4.0 root# vi tests/disclaim_test.c
82 void GC_CALLBACK pair_dct(void obj, void *cd)
83 {
84 pair_t p = obj;
85 int checksum;
86
87 / Check that obj and its car and cdr are not trashed. /
88 / # ifdef DEBUG_DISCLAIM_DESTRUCT /
89 / # endif /
90 my_assert(GC_base(obj));
91 my_assert(p->is_valid);
92 my_assert(!p->car || p->car->is_valid);
93 my_assert(!p->cdr || p->cdr->is_valid);
94 checksum = 782;
95 if (p->car) checksum += p->car->checksum;
96 if (p->cdr) checksum += p->cdr->checksum;
97 / if (p->checksum != checksum) { /
98 fprintf(stdout, "DEBUG: Destruct %p = (%p, %p)\n", (void *)p, (void *)p->car, (void *)p->cdr);
99 fprintf(stdout, "DEBUG: checksum %d:\n", checksum);
100 fprintf(stdout, "DEBUG: p->checksum %d:\n\n", p->checksum);
101 / } /
102 my_assert(p->checksum == checksum);
103
104 / Invalidate it. */
105 p->is_valid = 0;
106 p->checksum = 0;
107 p->car = cd;
108 p->cdr = NULL;
109 }
—
Reply to this email directly or view it on GitHub

from bdwgc.

Hi Ivan,

I think I managed to reproduce it even on x64:
https://github.com/paurkedal/bdwgc/tree/disclaim-test-magic
My suspicion is that there is something wrong with MARK_UNCONDITIONALLY, because when enabling the debugging, I can see a node being finalized after it's cdr.

Note that I'm busy at a conference this week, but I can take a closer look at this in the weekend if I don't get time before.

Best regards,
Petter

from bdwgc.

Also another thing for portability (I didn't check for this yet) is to declare variables in structs and functions in biggest to smallest order, i.e. pointers and longs through to ints and then chars. But since I'm not seeing segvs it's likely not the issue.

SPARC cpus are quite picky about alignment, x86's have an auto-coping mechanism

from bdwgc.

Hi Ivan,

I introduced the bug in https://github.com/ivmai/bdwgc/commits/63fd11df9468ac94373ba74e7bd4a97b9cc4044b. I must have forgotten why I placed the link to the finalizer closure in the first word of the object.
I put the fix, along with an improved test case in https://github.com/paurkedal/bdwgc/tree/fix-finalized-malloc, which I just rebased. I don't see a way to attach a pull request to the current discussion, but I can create a new one if you prefer.

Best regards,
Petter

from bdwgc.

In case anyone was looking at the fix-finalized-malloc branch I just force-updated it with a call to

GC_register_displacement_inner(sizeof(GC_word));

to support GC_set_all_interior_pointers(0).

from bdwgc.

Commit 63fd11d says: This way, we don't require all interior pointers, and GC_base returns the expected address.
Your recent change supports all_interior_pointers=0 but no correct GC_base. Could you please explain why it is impossible to put fclos at the end?

from bdwgc.

The reason is that the lowest bits of the first word is used to distinguish a previously freed object from one which just became free in order to decide whether or not to run the finalizer. Other object kinds don't have this issues, as they don't run any effect in relation to re-building free-lists. The old free-list is discarded, so whether an object was previously freed or just become free does not matter.

Now, we could make the same distinction using the last word, but the first-word assumption is used in mark.c:1808 (GC_push_unconditionally) which is the real bug if you look at it from that point of view. So, the question is what the API should be, considering that we're not only speaking of GC_finalized_malloc. I see the disclaim functionality as three distinct features:

• The possibility of registering a type with a callback which is invoked on unreachable objects.
• The option to trace from the same object unconditionally, to make sure the above notifier sees only valid pointers.
• The GC_finalized_malloc built on top of these two.

I mainly use the first to implement hash-consing.

From a functionality point if view, we could in fact drop the condition at mark.c:1808, but it would mean pushing free objects onto the mark stack. That probably has significant performance impact. I'm wondering if there is a better solution.

from bdwgc.

A way to solve the issue in a cleaner way would be to add a bit-vector to the heap block headers for object kinds containing disclaim notifiers, so that we can keep track of which objects are in use. This would also avoid calls to the notifier on unused objects. In order not increase the header size for other object kinds, either GC_install_header would need to be told the correct size to allocate or the header would contain a pointer to the extra bit-map allocated separately.

from bdwgc.

Fix commit merged to master (with small style-related modifications) - commit 2a3cef3
Test commit merged - commit 7b0be48

from bdwgc.

Thanks, I think this was the best way to fix it, despite the GC_base issue.

I tried to introduce "live" bits to track which objects where free. However, this approach increases the heap block header size my a non-negligible amount, and needed somewhat intrusive changes to malloc.c and mallocx.c.

from bdwgc.