-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i#2283: Facilitate Umbra Layouts with Constraint Solving. #2300
Conversation
I was going to say, don't we already have some of this: but it looks like the app I have is not checked in anywhere. It doesn't have a sat checker (used manual loops in the past). It does, however, check all the scales (up 2x, down 2x, down 4x, down 8x) for any particular setting, if you could add that here. I put it at #2301 (not to commit it just for comparison). The checked-in test we have is umbra_client_allscales but it only checks the checked-in config. |
So the script already allows one to check a layout for a given scale. I can add support to the script to check all scales but I wonder what the benefit is because Umbra associates an individual layout setting for each scale. |
That's not how I remember Umbra working: it tries to use the same setting for all scales, and only grudgingly makes an exception with a different displacement, having to do it just for one (or 2?) of the scales. Ideally there would be no exceptions, so I would want to see the results for all scales at once, preferring solutions with no exceptions. |
Okay, no problem. I added the new option.
I think the current implementation of Umbra tries the same setting for all scales by design, but on Linux, a different displacement value is used for each scale: Line 329 in 2d1e0a5
Interestingly, the script manages to synthesize a displacement value that should be suitable for all scales, assuming only one shadow map is registered.
If I try to consider additional maps, separated by 2 units, a single setting for all scales seems to be unsat on Linux.
|
@johnfxgalea -- is this mergeable? It may be very useful for #2328 where shadow layout problems are blocking us. |
Yep, it is ready to be merged but still awaiting a review I think. |
Oh, whoops. Though no longer sure #2328 is really a layout issue: may instead be an initialization/address walk issue at init time. |
Done. PTAL. I added some further comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Unfotunately the CI has been in bad shape for a while...I moved unix to Github Actions, and am trying to move Windows there and make it green too, but there are many problems to resolve.)
Xref #1712: we're hitting umbra shadow mapping failures up front on Linux on Github Actions. |
I thought I got Windows green -- are those flakes? Add to the ignore list? |
Does not seem to be a flake, with the following stable error:
|
@derekbruening Any idea what the above error is about please? Happy to investigate further, but just wondering if you have any idea? |
If that's downloading doxygen is it related to DynamoRIO/dynamorio#4654? |
No. Dr Memory's CI is making use of a stale URL (http://doxygen.nl/files/doxygen-1.8.19.windows.x64.bin.zip) here: drmemory/.github/workflows/ci-windows.yml Line 64 in 04b22ef
|
Ah yeah it needs the equiv of PR DynamoRIO/dynamorio#4643 |
Adds a helper python script to facilitate the setting up of Umbra Shadow Memory Layouts. Given the details of a memory layout, such as mask and disp values, the script will check whether any collisions are present with respect to the current pre-defined app regions that Umbra 64-bit considers today.
Example:
The script also uses a constraint solver (namely Z3) to check a layout as well as synthesize satisfying displacement values that result in no collisions (if sat).
Example: