Reland "vr: add WebXR-over-OpenXR support on Linux" [chromium/src : main]
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
#include "base/compiler_specific.h"I will verify whether it is necessary again.
Related details
- Alexander Cooper
- Brandon Jones
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper added 3 comments![]( "Open in Gerrit")
- device/vr/openxr/openxr_extension_helper.h: hold each OpenXR extensionDid these fail locally? It didn't seem like they failed on the bot; but I think the bot isn't neccessarily configured to run the xr_browser_tests yet, just the unittests you added.
#define OPENXR_LOAD_FN(name) \FWIW if you look for I think this macro name, we have a couple of other places we extract the extension functions out. If you could help update those to prevent future errors that'd be fantastic. I think there's a lighting estimator and maybe the spatial entities helper? We just don't have tests for them at the moment.
do { \Can you update the comment above too?
Related details
- Brandon Jones
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
- device/vr/openxr/openxr_extension_helper.h: hold each OpenXR extensionDid these fail locally? It didn't seem like they failed on the bot; but I think the bot isn't neccessarily configured to run the xr_browser_tests yet, just the unittests you added.
I think I use the wrong approach. I am trying to use ignore to see whether they can cover these APIs.
Related details
- Alexander Cooper
- Brandon Jones
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 2 comments![]( "Open in Gerrit")
Changes since the original CL, all specific to CFI (is_cfi) builds:utzcozFYI in case this happens in the future; typically the preferred way to do this would be to hit the "reland" button on the original CL, which will create a Patchset 1 that is identical to what was previously landed and *then* upload fixes as a Patchset 2 (or N), this allows an easy diffing of what you had to change to attempt the reland.
utzcozGot.I will care about it next time, although I don't hope next reland 😊.
Done
// CFI-checked (same approach as gpu/vulkan's VulkanFunction).utzcozAny chance any of this can directly be re-used here?
utzcozWill take look at it tonight.
The blocker is BindInstanceFunctionPointers()'s DCHECK_GE(api_version, VK_API_VERSION_1_1): we create the VkInstance with the version OpenXR negotiates (minApiVersionSupported, 1.0 for the runtimes I've tested) and the CFI bot builds dcheck_always_on. It's also a process-global singleton, so OpenXrVulkanContext would no longer own its table. Happy to switch if we're comfortable requiring Vulkan 1.1 from the runtime — just want your call before changing behavior.
Alexander Cooper (Gerrit)
Alexander Cooper added 2 comments![]( "Open in Gerrit")
FYI, That test failure feels unrelated.
// CFI-checked (same approach as gpu/vulkan's VulkanFunction).utzcozAny chance any of this can directly be re-used here?
utzcozWill take look at it tonight.
The blocker is BindInstanceFunctionPointers()'s DCHECK_GE(api_version, VK_API_VERSION_1_1): we create the VkInstance with the version OpenXR negotiates (minApiVersionSupported, 1.0 for the runtimes I've tested) and the CFI bot builds dcheck_always_on. It's also a process-global singleton, so OpenXrVulkanContext would no longer own its table. Happy to switch if we're comfortable requiring Vulkan 1.1 from the runtime — just want your call before changing behavior.
I'll defer to @baj...@chromium.org. It's not clear to me if we probably actually should enforce that minimum version to avoid errors with interfacing with Vulkan elsewhere, or if that's just because of e.g. functions that that helper extracts. If we can't re-use it, it's not the end of the world, just a thought/hope.
Related details
- Brandon Jones
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
I will verify whether it is necessary again.
- Alexander Cooper
- Brandon Jones
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brandon Jones (Gerrit)
Brandon Jones added 1 comment![]( "Open in Gerrit")
// CFI-checked (same approach as gpu/vulkan's VulkanFunction).utzcozAny chance any of this can directly be re-used here?
utzcozWill take look at it tonight.
Alexander CooperThe blocker is BindInstanceFunctionPointers()'s DCHECK_GE(api_version, VK_API_VERSION_1_1): we create the VkInstance with the version OpenXR negotiates (minApiVersionSupported, 1.0 for the runtimes I've tested) and the CFI bot builds dcheck_always_on. It's also a process-global singleton, so OpenXrVulkanContext would no longer own its table. Happy to switch if we're comfortable requiring Vulkan 1.1 from the runtime — just want your call before changing behavior.
I'll defer to @baj...@chromium.org. It's not clear to me if we probably actually should enforce that minimum version to avoid errors with interfacing with Vulkan elsewhere, or if that's just because of e.g. functions that that helper extracts. If we can't re-use it, it's not the end of the world, just a thought/hope.
Go ahead and require 1.1.
Consensus from everyone I've talked to so far is that Vulkan 1.0 isn't worth trying to support. The drivers are frequently very buggy and any device that will be reasonable to use in this context will have Vulkan 1.1 or greater.
Related details
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
// CFI-checked (same approach as gpu/vulkan's VulkanFunction).utzcozAny chance any of this can directly be re-used here?
utzcozWill take look at it tonight.
Alexander CooperThe blocker is BindInstanceFunctionPointers()'s DCHECK_GE(api_version, VK_API_VERSION_1_1): we create the VkInstance with the version OpenXR negotiates (minApiVersionSupported, 1.0 for the runtimes I've tested) and the CFI bot builds dcheck_always_on. It's also a process-global singleton, so OpenXrVulkanContext would no longer own its table. Happy to switch if we're comfortable requiring Vulkan 1.1 from the runtime — just want your call before changing behavior.
Brandon JonesI'll defer to @baj...@chromium.org. It's not clear to me if we probably actually should enforce that minimum version to avoid errors with interfacing with Vulkan elsewhere, or if that's just because of e.g. functions that that helper extracts. If we can't re-use it, it's not the end of the world, just a thought/hope.
Go ahead and require 1.1.
Consensus from everyone I've talked to so far is that Vulkan 1.0 isn't worth trying to support. The drivers are frequently very buggy and any device that will be reasonable to use in this context will have Vulkan 1.1 or greater.
- Alexander Cooper
- Brandon Jones
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 4 comments![]( "Open in Gerrit")
- device/vr/openxr/openxr_extension_helper.h: hold each OpenXR extensionutzcozDid these fail locally? It didn't seem like they failed on the bot; but I think the bot isn't neccessarily configured to run the xr_browser_tests yet, just the unittests you added.
I think I use the wrong approach. I am trying to use ignore to see whether they can cover these APIs.
Switched to the ignorelist so every call site is covered regardless; commit message corrected.
// CFI-checked (same approach as gpu/vulkan's VulkanFunction).utzcozAny chance any of this can directly be re-used here?
utzcozWill take look at it tonight.
Alexander CooperThe blocker is BindInstanceFunctionPointers()'s DCHECK_GE(api_version, VK_API_VERSION_1_1): we create the VkInstance with the version OpenXR negotiates (minApiVersionSupported, 1.0 for the runtimes I've tested) and the CFI bot builds dcheck_always_on. It's also a process-global singleton, so OpenXrVulkanContext would no longer own its table. Happy to switch if we're comfortable requiring Vulkan 1.1 from the runtime — just want your call before changing behavior.
Brandon JonesI'll defer to @baj...@chromium.org. It's not clear to me if we probably actually should enforce that minimum version to avoid errors with interfacing with Vulkan elsewhere, or if that's just because of e.g. functions that that helper extracts. If we can't re-use it, it's not the end of the world, just a thought/hope.
utzcozGo ahead and require 1.1.
Consensus from everyone I've talked to so far is that Vulkan 1.0 isn't worth trying to support. The drivers are frequently very buggy and any device that will be reasonable to use in this context will have Vulkan 1.1 or greater.
Got it. Will try it this weekend.
Done. Now goes through gpu::VulkanFunctionPointers (BindUnassociatedFunctionPointersFromLoaderLib / BindInstanceFunctionPointers / BindDeviceFunctionPointers) instead of a private table. Verify it with RADV.
FWIW if you look for I think this macro name, we have a couple of other places we extract the extension functions out. If you could help update those to prevent future errors that'd be fantastic. I think there's a lighting estimator and maybe the spatial entities helper? We just don't have tests for them at the moment.
This file is back to upstream — I dropped the per-call-site change for a directory entry in tools/cfi/ignores.txt (src:*device/vr/openxr/*).
Can you update the comment above too?
No longer applies — reverted this file, see above.
Related details
- Alexander Cooper
- Brandon Jones
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
I have done necessary tests including CFI failed test suites and functions, and I think it's ready for next loop review from my perspective, including new added CFI ignore text file.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper voted![]( "Open in Gerrit")
- Brandon Jones
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
Rebase and resolve conflicts. I have verified feature manually, and wait building to verify important tests as it consumes so long time on my machine to build tests. But I think it might be ready for reviewing now. Hi @vital...@chromium.org, @tha...@chromium.org, it might need your review for cfi ignore file changes. Thanks.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brandon Jones (Gerrit)
Brandon Jones voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brandon Jones
| Code-Review | +1 |
1 comment
LGTM. Use of gpu::GetVulkanFunctionPointers really helps clean this up, thanks!
Related details
- Alexander Cooper
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
Update with a double-close crash. PTAL.
a double-close crash fixing.
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
patchset 12 fixes multiple-xr-instance issue after rebasing.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Cooper (Gerrit)
Alexander Cooper voted Code-Review+1![]( "Open in Gerrit")
- Brandon Jones
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vitaly Buka (Gerrit)
Vitaly Buka added 1 comment![]( "Open in Gerrit")
# The OpenXR loader and Chromium's OpenXR code call into the dlopen'd OpenXR
# runtime through function pointers, like the vulkan loader above. This covers
# the loader, the generated dispatch table it dispatches through, and the
# extension entry points resolved with xrGetInstanceProcAddr.
src:*third_party/openxr/src/src/*
src:*device/vr/openxr/*it could be nicer to localize those dl calls into a few files
Related details
- Brandon Jones
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vitaly Buka (Gerrit)
Vitaly Buka added 1 comment![]( "Open in Gerrit")
Looks like I don't have permission to vote from @google.com
and I can't see this from @chromium.org account.
Vitaly Buka (Gerrit)
Vitaly Buka voted![]( "Open in Gerrit")
- Brandon Jones
- Nico Weber
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
Could you help to re-trigger CQ? Looks like failed cases are not related to this CL. And tree status becomes green after CQ.
Related details
- Brandon Jones
- Nico Weber
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
# The OpenXR loader and Chromium's OpenXR code call into the dlopen'd OpenXR
# runtime through function pointers, like the vulkan loader above. This covers
# the loader, the generated dispatch table it dispatches through, and the
# extension entry points resolved with xrGetInstanceProcAddr.
src:*third_party/openxr/src/src/*
src:*device/vr/openxr/*it could be nicer to localize those dl calls into a few files
Split. PTAL. I hope cfi test suite can be added for next CQ running.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
The patchset 13 also rebases and fixes openxr_test_helper.cc's CI failures with adding [[maybe_unused]].
Alexander Cooper (Gerrit)
Alexander Cooper voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexander Cooper
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
src:*device/vr/openxr/openxr_hand_tracker.ccIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Related details
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
What I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
What I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
In that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Related details
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
- utzcoz
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
In that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
I agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Vitaly Buka (Gerrit)
Vitaly Buka added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
I agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Oh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
Related details
- Alexander Cooper
- Brandon Jones
- Kentaro Hara
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vitaly Buka (Gerrit)
Vitaly Buka voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Alexander Cooper
- Brandon Jones
- Kentaro Hara
- Nico Weber
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Vitaly BukaI agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Oh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
Happy to take a TODO if we feel it's necessary to make some autogenerated stubs of the core functionality and switching the extension handler to a wrapped function paradigm.
Related details
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Vitaly BukaI agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Alexander CooperOh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
Happy to take a TODO if we feel it's necessary to make some autogenerated stubs of the core functionality and switching the extension handler to a wrapped function paradigm.
What about keeping this style of ignore file changes? Is it possible to submit this base commit if possible? I can address similar issues if you want to experiment. Hi @baj...@chromium.org, could you help to take look at it?
Related details
- Alexander Cooper
- Brandon Jones
- Kentaro Hara
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Kentaro Hara (Gerrit)
Kentaro Hara added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Vitaly BukaI agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Alexander CooperOh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
utzcozHappy to take a TODO if we feel it's necessary to make some autogenerated stubs of the core functionality and switching the extension handler to a wrapped function paradigm.
What about keeping this style of ignore file changes? Is it possible to submit this base commit if possible? I can address similar issues if you want to experiment. Hi @baj...@chromium.org, could you help to take look at it?
Do we need to add all of these files to ignores.txt? I prefer minimizing the list to 1) avoid overly disabling CFI and 2) reduce clean-up work we have to do in the future.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
- utzcoz
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Vitaly BukaI agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Alexander CooperOh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
utzcozHappy to take a TODO if we feel it's necessary to make some autogenerated stubs of the core functionality and switching the extension handler to a wrapped function paradigm.
Kentaro HaraWhat about keeping this style of ignore file changes? Is it possible to submit this base commit if possible? I can address similar issues if you want to experiment. Hi @baj...@chromium.org, could you help to take look at it?
Do we need to add all of these files to ignores.txt? I prefer minimizing the list to 1) avoid overly disabling CFI and 2) reduce clean-up work we have to do in the future.
The early version is:
src:*third_party/openxr/src/src/*
src:*device/vr/openxr/*
Which one do you prefer?
Related details
- Alexander Cooper
- Brandon Jones
- Kentaro Hara
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Kentaro Hara (Gerrit)
Kentaro Hara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kentaro Hara
| Code-Review | +1 |
1 comment
If the current file list is the minimized one, this CL LGTM.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
- utzcoz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
src:*device/vr/openxr/openxr_hand_tracker.ccutzcozIs it all callsites that need to be flagged or just the places that e.g. call xrGetInstanceProcAddr?
Alexander CooperWhat I understand is: all callsites of the resolved pointers — cfi-icall checks each indirect call. xrGetInstanceProcAddr itself is a direct call into the statically linked loader, so those places don't need flagging.
utzcozIn that case @vital...@chromium.org I feel that not doing device/vr/openxr/* is kind of just a lurking/lingering footgun for future changes to get tripped by the CFI, scoping to just the OpenXr runtime code already *is* fairly scoped as this is the code meant to talk to that runtime.
Vitaly BukaI agree with you. If vitaly can accept it, I can restore the previous version with device/vr/openxr/*.
Alexander CooperOh, now I see that chromium is sloppy here, the file is full of directory level suppression, other CFI deployments they track each suppression more seriously.
LGTM here either way.
@har...@chromium.org Maybe Chrome Memory Safety team should take ownership on this ignore list?
utzcozHappy to take a TODO if we feel it's necessary to make some autogenerated stubs of the core functionality and switching the extension handler to a wrapped function paradigm.
Kentaro HaraWhat about keeping this style of ignore file changes? Is it possible to submit this base commit if possible? I can address similar issues if you want to experiment. Hi @baj...@chromium.org, could you help to take look at it?
utzcozDo we need to add all of these files to ignores.txt? I prefer minimizing the list to 1) avoid overly disabling CFI and 2) reduce clean-up work we have to do in the future.
Kentaro HaraThe early version is:
src:*third_party/openxr/src/src/*
src:*device/vr/openxr/*
Which one do you prefer?
If the current file list is the minimized one, this CL LGTM.
Thanks. I think yes, it is the minimized list from my perspective. Each entry was found by auditing for indirect calls through resolved XR/Vulkan function pointers. Verified with a local CFI build (is_cfi + use_cfi_icall): device_unittests and xr_browser_tests pass with exactly these entries and crash without them.
Related details
- Alexander Cooper
- Brandon Jones
- Nico Weber
- Vitaly Buka
- Vitaly Buka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
utzcoz (Gerrit)
utzcoz added 1 comment![]( "Open in Gerrit")
Thanks for reviewing. I am ready for reland of this CL.