hotp_vul_table_lock is locked before shared_vm_areas

[groleo]

Signed-off-by: Adrian Negreanu [email protected]

[derekbruening]

Thank you for contributing. However:

1. Please see https://github.com/DynamoRIO/dynamorio/blob/master/CONTRIBUTING.md -- we need the CLA signed prior to accepting contributions.

2. We are using a rebase workflow with patches being reviewed on Rietveld.

3. For this particular patch, more information including a callstack of the lock order violation would help to understand the context and when this happens. What app and with what options are you running in order to hit this? Perhaps filing an issue in our tracker with this information would be best.

4. It does not look like the proposed solution is correct as it violates other constraints (e.g., "tracedump_mutex > shared_vm_areas").

[groleo]

Thank you for taking a look at the patch.
Indeed, after I looked at the comments, the change was not correct.

I tried to run make test on master, but it fails on linux, w/o any
patches.
Given this, i can't use it as a way to detect regressions.
What other way can I use to see if my patch breaks anything ?
Also, some of the LOCK_RANK in utils.h are not commented (like
LOCK_RANK(shared_cache_count_lock) )
What assumption is true for those ?

One more thing. Trying to access the review site is a loop :)
I go to https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews
which links to Rietveld code review site
https://github.com/DynamoRIO/dynamorio/wiki/codereview.appspot.com
Following that, leads me to https://github.com/DynamoRIO/dynamorio/wiki
If I click on Code-Reviews, I'm back where I started.

On Mon, Dec 15, 2014 at 8:06 PM, derekbruening [email protected]
wrote:

Thank you for contributing. However:

1. Please see
   https://github.com/DynamoRIO/dynamorio/blob/master/CONTRIBUTING.md -- we
   need the CLA signed prior to accepting contributions.

2. We are using a rebase workflow with patches being reviewed on Rietveld.

3. For this particular patch, more information including a callstack of
   the lock order violation would help to understand the context and when this
   happens. What app and with what options are you running in order to hit
   this? Perhaps filing an issue in our tracker with this information would be
   best.

4. It does not look like the proposed solution is correct as it violates
   other constraints (e.g., "tracedump_mutex > shared_vm_areas").

—
Reply to this email directly or view it on GitHub
#1572 (comment).

Regards!
http://groleo.wordpress.com

[derekbruening]

I tried to run make test on master, but it fails on linux, w/o any
patches.

"fails" in what way? What tests fail? We have tried to mark the handful of tests that have been observed as failing in some environments as _FLAKY in the names.

Given this, i can't use it as a way to detect regressions.
What other way can I use to see if my patch breaks anything ?

"make test" in a single build is never sufficient in any case. The full test suite must be run.

Providing some context in the issue tracker as originally requested would help. If you are running hotpatch code, we unfortunately do not have good tests of that, and it is not well-maintained and may suffer from bitrot. We have an open project proposal for someone to revive and expand our "probe mode" based on the hotpatching code.

One more thing. Trying to access the review site is a loop :)
I go to https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews
which links to Rietveld code review site
https://github.com/DynamoRIO/dynamorio/wiki/codereview.appspot.com

The GitHub wiki was auto-converted from our prior GoogleCode wiki and we are still straightening out all the links. That should be an absolute, not a relative, URL of course.

[groleo]

On Wed, Dec 17, 2014 at 9:46 PM, derekbruening [email protected]
wrote:

I tried to run make test on master, but it fails on linux, w/o any
patches.

"fails" in what way? What tests fail? We have tried to mark the handful of
tests that have been observed as failing in some environments as _FLAKY in
the names.

I've attached the output of make test. I assumed from the last 3 lines,
that
something went wrong and it's not something caused by tests that were
supposed to fail.
Errors while running CTest
Makefile:117: recipe for target 'test' failed
make: *** [test] Error 8

Given this, i can't use it as a way to detect regressions.
What other way can I use to see if my patch breaks anything ?

"make test" in a single build is never sufficient in any case. The full
test suite must be run.

I thought I might begin with the short one rather than wait for the full
suite.

Providing some context in the issue tracker as originally requested would
help. If you are running hotpatch code, we unfortunately do not have good
tests of that, and it is not well-maintained and may suffer from bitrot. We
have an open project proposal for someone to revive and expand our "probe
mode" based on the hotpatching code.

I'm trying to enable the probe API on Linux. I'm aware it should only run
on Windows;
Also I've seen a lot of what looks like issue ID's, but so far I don't know
where to find those, let alone filling new ones :)

As a "test", I'm using a modified insert_liboffs.client.c (
https://github.com/groleo/dbi-experiments/blob/master/probe-api/insert_liboffs.client.c
)
plus some other minor modifications in core/

One more thing. Trying to access the review site is a loop :)
I go to https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews
which links to Rietveld code review site
https://github.com/DynamoRIO/dynamorio/wiki/codereview.appspot.com

The GitHub wiki was auto-converted from our prior GoogleCode wiki and we
are still straightening out all the links. That should be an absolute, not
a relative, URL of course.

I got the link from the last commit on master
https://codereview.appspot.com/187360043

thanks a lot for your time.

Edit: moved the test-results.txt to pastebin : http://pastebin.com/fNTF4PKT
Github just inlined the whole file that I attached over email.

[derekbruening]

Something basic is off as nearly every test fails. Look at the details to see what's going on (run just one test and look at its output: "ctest -V -R common.eflags").