Page MenuHomePhabricator

[XRay] basic mode PID and TID always fetch
ClosedPublic

Authored by Maknee on Jul 6 2018, 7:57 AM.

Details

Summary

XRayRecords now includes a PID field. Basic handlers fetch pid and tid each time they are called instead of caching the value. Added a testcase that calls fork and checks if the child TID is different from the parent TID to verify that the processes' TID are different in the trace.

Event Timeline

Maknee created this revision.Jul 6 2018, 7:57 AM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptJul 6 2018, 7:57 AM
Maknee retitled this revision from XRay basic mode PID and TID always fetch to [XRay] basic mode PID and TID always fetch.Jul 6 2018, 7:59 AM
dberris requested changes to this revision.Jul 9 2018, 1:31 AM

Thanks @Maknee -- can you remove the 'TID' from thread-local data for basic mode logging to ensure that we've removed all references to it?

This revision now requires changes to proceed.Jul 9 2018, 1:31 AM
dberris added inline comments.Jul 9 2018, 1:35 AM
compiler-rt/lib/xray/xray_basic_logging.cc
229

Can you use internal_getpid() instead?

Maknee updated this revision to Diff 154613.Jul 9 2018, 8:19 AM
Maknee edited the summary of this revision. (Show Details)
Maknee updated this revision to Diff 154621.EditedJul 9 2018, 9:11 AM

Updating D49025: [XRay] basic mode PID and TID always fetch

Accidently added changes from different diff containing fdr changes

Maknee updated this revision to Diff 154641.Jul 9 2018, 10:46 AM

Replaced getpid() with internal_getpid(). Removed TID field from TLD and removed references to the TID field.

dberris accepted this revision.Jul 9 2018, 5:26 PM
dberris added a subscriber: kpw.

LGTM, with one minor comment.

Please let me know if/when it's ready to land, so that either I or @kpw can land it for you.

Thanks!

Cheers

compiler-rt/include/xray/xray_records.h
99

Minor: Maybe name this PId to be consistent with TId.

This revision is now accepted and ready to land.Jul 9 2018, 5:26 PM
Maknee updated this revision to Diff 154846.Jul 10 2018, 10:50 AM
Maknee marked an inline comment as done.

Changed Pid to PId in XRayRecord

Maknee accepted this revision.Jul 10 2018, 10:52 AM
Maknee marked an inline comment as done.

Ready to land

This revision was automatically updated to reflect the committed changes.
ormris added a subscriber: ormris.Jul 18 2018, 9:00 AM
krytarowski added a subscriber: krytarowski.EditedNov 5 2018, 6:08 PM

This test seems to have a Linux assumption and I have concerns how to port it to NetBSD.

On NetBSD TIDs start from 1 to N in each process. Each process has a single distinct PID. TID numbers are not unique between processes (they are unique only within a process). PID numbers are unique.

On Linux TID is effectively PID and this is linuxism.

This test seems to have a Linux assumption and I have concerns how to port it to NetBSD.

On NetBSD TIDs start from 1 to N in each process. Each process has a single distinct PID. TID numbers are not unique between processes (they are unique only within a process). PID numbers are unique.

On Linux TID is effectively PID and this is linuxism.

Right, I'm missing the concern here -- the change actually records both PID and TID by getting them regularly instead of caching them at the start of the thread.

Does this fail in NetBSD and if it does, what is the failure mode?

In this test parent_tid = 1, child_tid = 1 always and distinguishing this way child and parent is broken. Switching syscall_gettid() to behave like getpid() is not sufficient as the test has assumptions that thread is like pid.

$ /public/llvm-build/./bin/llvm-xray convert --symbolize --output-format=yaml -instr_map=/public/llvm-build/projects/compiler-rt/test/xray/X86_64NetBSDConfig/TestCases/Posix/Output/fork_basic_logging.cc.tmp      "`ls -S fork-basic-logging-test-* | head -1`"      | ./bin/FileCheck /public/compiler-rt/test/xray/TestCases/Posix/fork_basic_logging.cc --check-prefix=TRACE
---
header:          
  version:         3
  type:            0
  constant-tsc:    true
  nonstop-tsc:     true
  cycle-frequency: 3392489040
records:         
  - { type: 0, func-id: 1, function: 'log_syscall_gettid()', cpu: 0, thread: 1, process: 6843, kind: function-enter, tsc: 4422386299208 }
  - { type: 0, func-id: 1, function: 'log_syscall_gettid()', cpu: 0, thread: 1, process: 6843, kind: function-enter, tsc: 4422386299208 }
  - { type: 0, func-id: 1, function: 'log_syscall_gettid()', cpu: 0, thread: 1, process: 6843, kind: function-exit, tsc: 4422386409791 }
  - { type: 0, func-id: 1, function: 'log_syscall_gettid()', cpu: 0, thread: 1, process: 6843, kind: function-exit, tsc: 4422386409791 }
  - { type: 0, func-id: 4, function: 'print_parent_or_child()', cpu: 0, thread: 1, process: 6843, kind: function-enter, tsc: 4422386596150 }
  - { type: 0, func-id: 2, function: 'print_parent_tid(unsigned long)', args: [ 6843 ], cpu: 0, thread: 1, process: 6843, kind: function-enter-arg, tsc: 4422386614652 }
  - { type: 0, func-id: 2, function: 'print_parent_tid(unsigned long)', cpu: 0, thread: 1, process: 6843, kind: function-exit, tsc: 4422386648144 }
  - { type: 0, func-id: 4, function: 'print_parent_or_child()', cpu: 0, thread: 1, process: 6843, kind: function-exit, tsc: 4422386648972 }
  - { type: 0, func-id: 4, function: 'print_parent_or_child()', cpu: 0, thread: 1, process: 14316, kind: function-enter, tsc: 4422387049656 }
  - { type: 0, func-id: 3, function: 'print_child_tid(unsigned long)', args: [ 14316 ], cpu: 0, thread: 1, process: 14316, kind: function-enter-arg, tsc: 4422387125834 }
  - { type: 0, func-id: 3, function: 'print_child_tid(unsigned long)', cpu: 0, thread: 1, process: 14316, kind: function-exit, tsc: 4422387294637 }
  - { type: 0, func-id: 4, function: 'print_parent_or_child()', cpu: 0, thread: 1, process: 14316, kind: function-exit, tsc: 4422387295979 }
...
/public/compiler-rt/test/xray/TestCases/Posix/fork_basic_logging.cc:94:15: error: TRACE-DAG: expected string not found in input
// TRACE-DAG: - { type: 0, func-id: [[PPTARG:[0-9]+]], function: {{.*print_parent_tid.*}}, args: [ [[THREAD1]] ], cpu: {{.*}}, thread: [[THREAD1]], process: [[PROCESS1]], kind: function-enter-arg, tsc: {{[0-9]+}} }
              ^
<stdin>:1:1: note: scanning from here
---
^
<stdin>:1:1: note: with variable "THREAD1" equal to "1"
---
^
<stdin>:1:1: note: with variable "THREAD1" equal to "1"
---
^
<stdin>:1:1: note: with variable "PROCESS1" equal to "6843"
---
^

I've marked this test as UNSUPPORTED for NetBSD. It would be nice to keep a test for TID that is portable, but it would need significant rewrite.

Aha! Right, I think the test is wrong indeed. We shouldn't be assuming that the tid == pid in the test, when recording the first argument for the thread id, and rather be calling the correct function for getting the thread id all the time.