Replace DMD's custom TLS code with use of mozilla/ThreadLocal.h

RESOLVED FIXED in Firefox 68

Status

()

enhancement
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla68
Points:
---

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

DMD predates mozilla/ThreadLocal.h.

Assignee

Comment 2

3 months ago

erahm: review ahoy!

Flags: needinfo?(erahm)

Comment 3

3 months ago

(In reply to Nicholas Nethercote [:njn] from comment #2)

erahm: review ahoy!

Thanks for ni! I should probably remove that now that we're a phab-only shop.

Flags: needinfo?(erahm)

Comment 4

2 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46f6705f9c0c
Replace DMD's custom TLS code with use of mozilla/ThreadLocal.h. r=erahm

Backed out changeset 46f6705f9c0c (bug 1533240) for xpcshell failures at /test/test_dmd.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/bc61185b6f5747b5b40b6c9639972a8003543834

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=46f6705f9c0c2c8bbd45b60615a1081f40a8402b&selectedJob=233799763

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=233799763&repo=autoland&lineNumber=7762

Log snippet:

04:30:08 INFO - TEST-START | memory/replace/dmd/test/test_dmd.js
04:30:09 WARNING - TEST-UNEXPECTED-FAIL | memory/replace/dmd/test/test_dmd.js | xpcshell return code: 0
04:30:09 INFO - TEST-INFO took 744ms
04:30:09 INFO - >>>>>>>
04:30:09 INFO - PID 7337 | [7337, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2528
04:30:09 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
04:30:09 INFO - PID 7337 | DMD[7338] $DMD = '1'
04:30:09 INFO - PID 7337 | Traceback (most recent call last):
04:30:09 INFO - PID 7337 | File "/Users/cltbld/tasks/task_1552535172/build/application/Firefox NightlyDebug.app/Contents/Resources/dmd.py", line 916, in <module>
04:30:09 INFO - PID 7337 | main()
04:30:09 INFO - PID 7337 | File "/Users/cltbld/tasks/task_1552535172/build/application/Firefox NightlyDebug.app/Contents/Resources/dmd.py", line 908, in main
04:30:09 INFO - PID 7337 | digest = getDigestFromFile(args, args.input_file)
04:30:09 INFO - PID 7337 | File "/Users/cltbld/tasks/task_1552535172/build/application/Firefox NightlyDebug.app/Contents/Resources/dmd.py", line 274, in getDigestFromFile
04:30:09 INFO - PID 7337 | fixStackTraces(inputFile, isZipped, opener)
04:30:09 INFO - PID 7337 | File "/Users/cltbld/tasks/task_1552535172/build/application/Firefox NightlyDebug.app/Contents/Resources/dmd.py", line 258, in fixStackTraces
04:30:09 INFO - PID 7337 | with opener(inputFilename, 'rb') as inputFile:
04:30:09 INFO - PID 7337 | IOError: [Errno 2] No such file or directory: '/Users/cltbld/tasks/task_1552535172/build/tests/xpcshell/tests/memory/replace/dmd/test/complete-empty-live.json'
04:30:09 INFO - PID 7337 | --- /Users/cltbld/tasks/task_1552535172/build/tests/xpcshell/tests/memory/replace/dmd/test/complete-empty-live-expected.txt 2016-01-01 00:00:00.000000000 +0000
04:30:09 INFO - PID 7337 | +++ /Users/cltbld/tasks/task_1552535172/build/tests/xpcshell/tests/memory/replace/dmd/test/complete-empty-live-actual.txt 2019-03-14 04:30:09.000000000 +0000
04:30:09 INFO - PID 7337 | @@ -1,18 +0,0 @@
04:30:09 INFO - PID 7337 | -#-----------------------------------------------------------------
04:30:09 INFO - PID 7337 | -# dmd.py --filter-stacks-for-testing -o complete-empty-live-actual.txt complete-empty-live.json
04:30:09 INFO - PID 7337 | -
04:30:09 INFO - PID 7337 | -Invocation {
04:30:09 INFO - PID 7337 | - $DMD = '--mode=live --stacks=full'
04:30:09 INFO - PID 7337 | - Mode = 'live'
04:30:09 INFO - PID 7337 | -}

Flags: needinfo?(n.nethercote)
Assignee

Comment 6

2 months ago

The problem on Mac is as follows.

  • Currently, DMD uses pthread_key_create(), pthread_getspecific(), and pthread_setspecific() for the TLS implementation.

  • ThreadLocal.h instead uses thread_local for Mac.

  • replace_malloc() is called for the first malloc() call. It calls Thread::Fetch().

  • Thread::Fetch() gets the TLS value. When using thread_local, this triggers a call to tlv_get_addr(), which calls tlv_allocate_and_initialize_for_key(), which calls malloc(), so we end up in replace_malloc()... and the pattern repeats.

  • So we get infinite recursion and stack overflow.

Presumably the pthread_* functions don't involve allocations, thus avoiding the problem.

I can't see how to avoid this problem. replace_malloc() simply can't call functions that are guaranteed to allocate. (Well, there's the whole intercept blocking thing; it might be ok when intercepts are blocked. But checking to see if intercepts are blocked requires accessing TLS!)

Flags: needinfo?(n.nethercote)
Assignee

Comment 7

2 months ago

glandium pointed me at https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/memory/build/mozjemalloc.cpp#1154-1164, which shows how mozjemalloc deals with this problem -- it just uses pthread-based TLS on Mac.

albeit, using the mozilla/ThreadLocal.h code :)

Assignee

Comment 9

2 months ago

erahm: review ping! Only a minor change was necessary to fix the Mac problem, as discussed above.

Flags: needinfo?(erahm)
Assignee

Updated

2 months ago
Flags: needinfo?(erahm)

Comment 10

2 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40046966515d
Replace DMD's custom TLS code with use of mozilla/ThreadLocal.h. r=erahm

Comment 11

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.