Flesh out the initial tracing SHIM API. by vext01 · Pull Request #1 · softdevteam/yktrace

vext01 · 2019-01-29T12:04:46Z

This is a DNMY PR. I'm raising for comments on the API design of yktrace.

Please see the commit message for more info.

If we like the design, there's a change to ykrustc that needs to go in first, then this PR, then we should enable buildbot on this repo.

Cheers

vext01 · 2019-01-29T16:03:48Z

This is everything apart from the buildbot.

NOTE: we will have to revise the commit message before merge (handles -> tracers).

ltratt · 2019-01-30T10:36:11Z

+pub fn start_tracing(kind: TracingKind) -> Box<dyn ThreadTracer> {
+ match kind {
+ TracingKind::SoftwareTracing => swt::start_tracing(),
+ _ => panic!("tracing kind not implemented")


Should be unimplemented?

yeah, probably.

ltratt · 2019-01-30T10:38:06Z

+ }
+
+ fn loc(&self, idx: usize) -> &MirLoc {
+ if idx > isize::max_value() as usize {


Why are we checking against isize::max_value? Is it because we're using offset below? If so, add negates the need for this check (assuming, as I think we must, that ptr + self.len is in bounds).

Is it because we're using offset below?
Yeah, offset accepts an isize so that you can bump pointers both back and forward:
pub unsafe fn offset(self, count: isize) -> *const T
pub unsafe fn offset(self, count: isize) -> *const T
Ah! I did not know about Ptr::add! Marvellous. Will fix.

ltratt · 2019-01-30T10:38:41Z

+
+impl ThreadTracer for SWTThreadTracer {
+ fn stop_tracing(self: Box<Self>) -> Option<Box<dyn MirTrace>> {
+ match yk_swt::stop_tracing() {


Maybe yk_swt::stop_tracing().map(|(buf, len)| Some(Box::new(...))?

Ah yes. I never was good at all of these combinators :)

ltratt · 2019-01-30T10:45:29Z

+/// A thread tracer is returned through which tracing can be stopped and the trace obtained. The
+/// user must ensure that only one thread tracer exists at any one time per-thread, otherwise
+/// undefined behaviour will result.
+pub fn start_tracing(kind: TracingKind) -> Box<dyn ThreadTracer> {


Is there a function somewhere which says "start tracing using whatever the best available tracer is"?

I've not implemented one, no. We could have kind an Option where None means "just frigging choose one, will ya!".

I was thinking the same thing. Let's do it!

vext01 · 2019-01-30T11:29:39Z

The rustfmt hack I used yesterday no longer works. Trying to figure out how to do it properly...

vext01 · 2019-01-30T12:30:51Z

The rustfmt hack I used yesterday no longer works. Trying to figure out how to do it properly...

For future reference, to build rustfmt from x.py it's: ./x.py build src/tools/rustfmt then you have to run the tool manually, i.e.: ~/research/ykrustc_pv/build/x86_64-unknown-linux-gnu/stage2-tools-bin/rustfmt src/swt.rs.

You can't build rustfmt at stage1, and once it's built in this way you can't run it via cargo fmt either.

Ready for re-review.

vext01 · 2019-01-30T14:31:26Z

Ready.

ltratt · 2019-01-30T14:32:55Z

Please squash.

vext01 · 2019-01-30T14:33:50Z

I forgot to update the docstring. If it looks good, I'll squash.

ltratt · 2019-01-30T14:35:38Z

-/// A thread tracer is returned through which tracing can be stopped and the trace obtained. The
-/// user must ensure that only one thread tracer exists at any one time per-thread, otherwise
-/// undefined behaviour will result.
+/// By passing `kind` as `None` an appropriate tracing kind will be selected automatically. A


How about simplifying to something like "If None is passed, then an appropriate backend will be selected; if Some(...) is passed, a specific backend can be specified. Only one thread tracer can be started at any point; starting more than one leads to undefined behaviour."?

Try my last push. I want the restrictions on threads to be super-clear.

ltratt · 2019-01-30T15:02:47Z

-/// must ensure that only one thread tracer exists at any one time per-thread, otherwise undefined
-/// behaviour will result.
+/// If `None` is passed, then an appropriate tracing kind will be selected; by passing `Some(...)`,
+/// a specific kind can be chosen. Tracing different threads concurrently is allowed, but tracing


The second sentence doesn't really help IMHO. Maybe something like "Any given thread can only have one active tracer; calling start_tracing on a thread where there is already an active tracer leads to undefined behaviour".

"can only have one active tracer" might better be "must have at most one active tracer"?

I wrote exactly the same independently!

vext01 · 2019-01-30T15:08:24Z

Try this.

ltratt · 2019-01-30T15:09:08Z

Please squash.

Although for now ykrustc only supports software tracing, I've designed the API assuming that there can be a runtime switch between different tracing backends (e.g. hardware/software tracing). Making ykrustc support such a switch is future work. For now the API supports: * Starting a trace and returning a thread tracer struct. * Stopping a trace, consuming the thread tracer (to prevent double-stop scenarios), and returning the recorded trace. * Getting a MIR location from a trace at a given index. * Iteration over a trace. The API also adds a Drop implementation for software traces so that users don't have to manually free a raw pointer at the correct time.

vext01 · 2019-01-30T15:13:13Z

Squashed and revised commit message.

Before we merge this, we need to review and merge: softdevteam/ykrustc#5

5: Pull the trace struct out of the low-level interface. r=ltratt a=vext01 This is a companion change to softdevteam/yktrace#1 Co-authored-by: Edd Barrett <vext01@gmail.com>

vext01 · 2019-01-31T10:14:53Z

With the ykrustc PR now merged we can now do this one.

My next PR will enable buildbot and bors on this repo.

ltratt · 2019-01-31T10:17:48Z

Why don't we do the buildbot/bors PR first, merge that into master; then we can merge master into this PR and use buildbot to check this PR?

vext01 · 2019-01-31T10:20:38Z

Why don't we do the buildbot/bors PR first

We could, I'd have to add some dummy code to build though.

ltratt · 2019-01-31T10:21:28Z

Maybe we should just add the buildbot stuff into this PR as a separate commit? It's fairly self-contained, right?

vext01 · 2019-01-31T10:22:49Z

Maybe we should just add the buildbot stuff into this PR as a separate commit? It's fairly self-contained, right?

We can try, but like i said the other day, I'm not sure if bors will see existing PRs. It's something I'd like to know, so no objections to trying.

ltratt · 2019-01-31T10:23:04Z

Let's try it then :)

vext01 · 2019-01-31T10:24:22Z

OK, it may take a little while, as I'm struggling to get a working rustfmt and rustdoc in our ykrustc image.

vext01 · 2019-01-31T10:59:01Z

bors ping

bors · 2019-01-31T10:59:03Z

pong

ltratt · 2019-01-31T11:09:06Z

It doesn't look like the build has started?

vext01 · 2019-01-31T11:09:37Z

I'm working on it.

vext01 · 2019-01-31T11:10:47Z

bors try-

vext01 · 2019-01-31T11:11:05Z

bors try

bors · 2019-01-31T11:13:19Z

try

Build failed

buildbot/buildbot-build-script

vext01 · 2019-01-31T11:15:18Z

bors try

vext01 · 2019-01-31T11:19:58Z

OK, hopefully this should work.

I made the mistake of informing the buildbot about the new repo after bors try, which meant buildbot saw the trying commit as "old news" and didn't bother trying to build it.

The build will take ages because we have to build a full stage2 compiler to have rustdoc and rustfmt (the build targets for these fail if you try to force stage 1 only).

In the meantime, are you happy with the CI commits?

ltratt · 2019-01-31T11:21:02Z

I sort-of assumed you'd want to squash the third commit (the merge) away? If so, do you want to squash now, then we can do a full build (an r plus build).

vext01 · 2019-01-31T11:22:56Z

I sort-of assumed you'd want to squash the third commit

Well yes, but you have to request squashes.

Since you are OK with it, I'll do it now.

bors · 2019-01-31T11:23:25Z

try

Build failed

buildbot/buildbot-build-script

vext01 · 2019-01-31T11:24:22Z

Squashed

ltratt · 2019-01-31T11:24:41Z

bors r+

1: Flesh out the initial tracing SHIM API. r=ltratt a=vext01 This is a DNMY PR. I'm raising for comments on the API design of yktrace. Please see the commit message for more info. If we like the design, there's a change to ykrustc that needs to go in first, then this PR, then we should enable buildbot on this repo. Cheers Co-authored-by: Edd Barrett <vext01@gmail.com>

bors · 2019-01-31T12:45:05Z

Build failed

buildbot/buildbot-build-script

vext01 · 2019-01-31T13:47:09Z

run pkg_config fail: "Failed to run `"pkg-config" "--libs" "--cflags" ...

This is due to missing packages on the buildbot machine. I've added them (for the record libssl-dev and pkg-config).

I've also added the config.toml options from ykrustc. Most importantly, use threads to build -- should be faster.

Shall I squash it in?

ltratt · 2019-01-31T14:10:13Z

Yes, please squash.

vext01 · 2019-01-31T14:11:17Z

splat

ltratt · 2019-01-31T14:12:50Z

bors r+

1: Flesh out the initial tracing SHIM API. r=ltratt a=vext01 This is a DNMY PR. I'm raising for comments on the API design of yktrace. Please see the commit message for more info. If we like the design, there's a change to ykrustc that needs to go in first, then this PR, then we should enable buildbot on this repo. Cheers Co-authored-by: Edd Barrett <vext01@gmail.com>

bors · 2019-01-31T22:42:30Z

Timed out

ltratt · 2019-01-31T22:56:35Z

Hmm, 9 hours is a long time. Let's try again.

bors r+

1: Flesh out the initial tracing SHIM API. r=ltratt a=vext01 This is a DNMY PR. I'm raising for comments on the API design of yktrace. Please see the commit message for more info. If we like the design, there's a change to ykrustc that needs to go in first, then this PR, then we should enable buildbot on this repo. Cheers Co-authored-by: Edd Barrett <vext01@gmail.com>

bors · 2019-02-01T01:09:15Z

Build succeeded

buildbot/buildbot-build-script

vext01 assigned ltratt Jan 29, 2019

vext01 mentioned this pull request Jan 29, 2019
Pull the trace struct out of the low-level interface. softdevteam/ykrustc#5
Merged

ltratt reviewed Jan 30, 2019
View reviewed changes

vext01 force-pushed the initial branch from 2d615da to 03e316dCompare January 30, 2019 15:12

borsBot added a commit to softdevteam/ykrustc that referenced this pull request Jan 30, 2019
Merge#5
1219d0c
5: Pull the trace struct out of the low-level interface. r=ltratt a=vext01 This is a companion change to softdevteam/yktrace#1 Co-authored-by: Edd Barrett <vext01@gmail.com>

borsBot added a commit to softdevteam/ykrustc that referenced this pull request Jan 30, 2019
Merge#5
03c5d50
5: Pull the trace struct out of the low-level interface. r=ltratt a=vext01 This is a companion change to softdevteam/yktrace#1 Co-authored-by: Edd Barrett <vext01@gmail.com>

borsBot added a commit that referenced this pull request Jan 31, 2019
Try#1:
77b145a

borsBot added a commit that referenced this pull request Jan 31, 2019
Try#1:
0873eaf

vext01 force-pushed the initial branch from 402f451 to cde4a92Compare January 31, 2019 11:24

Enable buildbot/bors.
f6f04f3

vext01 force-pushed the initial branch from 0503d02 to f6f04f3Compare January 31, 2019 14:11

borsBot merged commit f6f04f3 into masterFeb 1, 2019

borsBot deleted the initial branch February 1, 2019 01:09

Uh oh!

Conversation

vext01 commented Jan 29, 2019

Uh oh!

vext01 commented Jan 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

ltratt commented Jan 30, 2019

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

ltratt commented Jan 30, 2019

Uh oh!

vext01 commented Jan 30, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

ltratt commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

ltratt commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

ltratt commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

borsBot commented Jan 31, 2019

Uh oh!

ltratt commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

vext01 commented Jan 31, 2019

Uh oh!

borsBot commented Jan 31, 2019

try