New issue
Advanced search Search tips

Issue 851718 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 850728



Sign in to add a comment

Fix Vulkan platform define

Project Member Reported by cblume@chromium.org, Jun 11 2018

Issue description

Previously, the platform Vulkan define was being set in BUILD.gn.
In a refactor it moved into a platform header.

This required that platform header to be included if using that platform's code.


This caused a problem where the function pointer header would change a struct's definition based on what platform it is being built for. For example, if we are building for Android then we need a function pointer to the Android Vulkan init.

If one translation unit has the platform header included, it would see a struct with that Android Vulkan init function pointer. If a second translation unit didn't include the platform header, it would see a different struct -- one without the Android Vulkan init.

As a result of the two translation units seeing different structs the final program would have undefined behavior. In this case, the UB manifest itself as clobbering data. The Android Vulkan init function pointer would be clobbered by whatever function pointer came after it in the list.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab96bc86f1f6a627940fee0c03b0471e7d0fa126

commit ab96bc86f1f6a627940fee0c03b0471e7d0fa126
Author: Chris Blume <cblume@chromium.org>
Date: Tue Jun 12 20:35:54 2018

Fix Vulkan platform define

Different translation units were seeing different structs holding the
Vulkan function pointers. This was because those different translation
units may or may not have had the Vulkan platform define set.

This means one translation unit saw a struct which contained an
Android-specific function pointer. Another translation unit saw a struct
which didn't have that function pointer.

The function pointers were being clobbered as these different
translation units attempted to fill in the struct.

This patch will set the Vulkan platform define in BUILD.gn so all Vulkan
translation units have that define set.

BUG= 851718 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Idb56c9caaeaf9ac8351cd58fb9c70877cc909487
Reviewed-on: https://chromium-review.googlesource.com/1096373
Commit-Queue: Chris Blume <cblume@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566555}
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/BUILD.gn
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/vulkan_device_queue.cc
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/vulkan_implementation_android.cc
[delete] https://crrev.com/e9782cea1be5d36234abb3eb6ae7d09f78358330/gpu/vulkan/vulkan_platform.h
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/vulkan_surface.cc
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/x/BUILD.gn
[modify] https://crrev.com/ab96bc86f1f6a627940fee0c03b0471e7d0fa126/gpu/vulkan/x/vulkan_implementation_x11.cc

Comment 2 by cblume@chromium.org, Jun 12 2018

Status: Fixed (was: Assigned)

Sign in to add a comment