554860 - Sampler::takeSample should be volatile

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Mike Morearty]

In Sampler.h, "int32_t takeSample" should be "volatile int32_t takeSample", because that variable is set from one thread and accessed from another thread.  A compiler might optimize away the reads, thus causing the profiler not to take samples as often as it should.

It's an easy change, but in practice, this is highly unlikely because of the calling pattern, so I'm fine with deferring this until flash10.2.

Comment 1

[Mike Morearty]

Attachment: patch

Comment 2

[Edwin Smith]

Is it necessary to transmit the volatile keyword up to the OSDep API?  (unrelated:  is OSDep pretty much obsolete now with VMPI?)

Comment 3

[Mike Morearty]

(In reply to comment #2)
> Is it necessary to transmit the volatile keyword up to the OSDep API? 

I was wondering that too.  I don't really feel that it is necessary to transmit it; but I tried leaving the OSDep API unchanged, and then I got a compiler warning on the line of Sampler.cpp that calls OSDep::startIntWriterTimer() -- the compiler didn't like the fact that I was passing a (volatile int *) to a function that is expecting an (int *).  Obviously that would be easy to fix with a cast, but in general casts are somewhat undesirable.

I really don't have a strong opinion about it -- I'd be fine with putting in a cast if others think that's a better approach.

Comment 4

[Edwin Smith]

i dont have a strong opinion either, just odd.

Comment 5

[Edwin Smith]

Comment on attachment 443674 [details] [diff] [review]
patch

On second thought I think volatile is correct at the interface; the contract is that a background thread will write to *addr.

Comment 6

[Mike Morearty]

Pushed: http://hg.mozilla.org/tamarin-redux/rev/5d9d0b0e8be8