New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 728271 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Chrome , Mac
Pri: 2
Type: Bug


Sign in to add a comment

enable SH_INITIALIZE_UNINITIALIZED_LOCALS in the shader translator

Project Member Reported by kainino@chromium.org, May 31 2017

Issue description

oetuaho (https://bugs.chromium.org/p/angleproject/issues/detail?id=1966#c20):
> The implementation in ANGLE is now done. If initializing local variables is wanted in the old GL backend of Chromium on Linux, Chromium needs to enable SH_INITIALIZE_UNINITIALIZED_LOCALS in the shader translator. Alternatively the issue will take care of itself once Chromium starts to use the GL backend of ANGLE on Linux.


 
Components: -Internals>GPU>WebGL Blink>WebGL
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 18 2017

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

commit 62e46a335dbbde9d2008f78290b75dd5b9a9f52c
Author: Kai Ninomiya <kainino@chromium.org>
Date: Mon Sep 18 20:31:24 2017

Enable SH_INITIALIZE_UNINITIALIZED_LOCALS

Bug:  728271 ,  angleproject:1966 , angleproject:2041,  angleproject:2046 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Idf3aca674a16d034ff54ffd1db61665db0d142f2
Reviewed-on: https://chromium-review.googlesource.com/667969
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502662}
[modify] https://crrev.com/62e46a335dbbde9d2008f78290b75dd5b9a9f52c/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py
[modify] https://crrev.com/62e46a335dbbde9d2008f78290b75dd5b9a9f52c/content/test/gpu/gpu_tests/webgl_conformance_expectations.py
[modify] https://crrev.com/62e46a335dbbde9d2008f78290b75dd5b9a9f52c/gpu/command_buffer/service/shader_translator.cc

Blocking: -angleproject:1966
Blockedon: angleproject:1966
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 20 2017

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

commit 5596e20d477745d010c68959c8d931d00ae30991
Author: Brian Anderson <brianderson@chromium.org>
Date: Wed Sep 20 00:04:03 2017

Expect gles3_shaderindexing* to fail on some Macs

BUG= 728271 

Change-Id: I544a89f3fb8d58bc5afc914672e5cd8ee6e350c5
Reviewed-on: https://chromium-review.googlesource.com/673874
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Brian Anderson <brianderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502997}
[modify] https://crrev.com/5596e20d477745d010c68959c8d931d00ae30991/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 21 2017

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

commit 956f915cc29d7e7a94a5dc17ee784118d2466e36
Author: Brian Anderson <brianderson@chromium.org>
Date: Thu Sep 21 02:43:03 2017

Expect failure instead of flake for gles3_shaderindexing

BUG= 728271 
TBR=zmo@chromium.org

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I008a86f4504f8e83f37dedfc11c574d689193b8b
Reviewed-on: https://chromium-review.googlesource.com/676279
Commit-Queue: Brian Anderson <brianderson@chromium.org>
Reviewed-by: Brian Anderson <brianderson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503338}
[modify] https://crrev.com/956f915cc29d7e7a94a5dc17ee784118d2466e36/content/test/gpu/gpu_tests/webgl2_conformance_expectations.py

Cc: kainino@chromium.org zmo@chromium.org kbr@chromium.org ynovikov@chromium.org briander...@chromium.org jmad...@chromium.org
 Issue 767276  has been merged into this issue.

Comment 9 by kainino@google.com, Oct 5 2017

 Issue 771936  has been merged into this issue.

Comment 10 Deleted

Status: Started (was: Fixed)
Reopening this for now. We're still looking into ANGLE native test crashes in Android's shader compiler and may need to disable this on a few Android platforms.

We could also consider disabling it on Mac/NVIDIA but I think this patch is more important than the small regressions it added on that platform (which is not well anyway).
p.s.
android:  issue angleproject:2046 
mac/nvidia: issue angleproject:2041
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 6 2017

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

commit 4971543fc71fa2b61054f1b0443617ce6dea988d
Author: Kai Ninomiya <kainino@chromium.org>
Date: Fri Oct 06 22:13:56 2017

Adreno/M: Don't initialize uninitialized locals

Bug:  728271 ,  angleproject:2046 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I62ad71837354add82a7f5f3776bcbcda2ce052df
Reviewed-on: https://chromium-review.googlesource.com/704019
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507191}
[modify] https://crrev.com/4971543fc71fa2b61054f1b0443617ce6dea988d/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/4971543fc71fa2b61054f1b0443617ce6dea988d/gpu/command_buffer/service/shader_translator.cc
[modify] https://crrev.com/4971543fc71fa2b61054f1b0443617ce6dea988d/gpu/config/gpu_driver_bug_list.json
[modify] https://crrev.com/4971543fc71fa2b61054f1b0443617ce6dea988d/gpu/config/gpu_driver_bug_workaround_type.h

Since the uninitialized-locals-globals tests passed on the Nexus 5X bots (in the following commit), it seems like this workaround may not be strictly necessary there anyway. Makes things a bit easier. I didn't add in failure expectations so we'll continually test that assertion.

However if those tests start flaking on the 5X bots, we'll have to mark them as flaky or failing.
Blocking: 773294
Blockedon: 773294
Blocking: -773294
Fails on Nexus 6 now:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Android%20Release%20%28Nexus%206%29/builds/9841

Maybe workaround only on "Adreno (TM) 418" - Nexus 5X?
Nexus 6 is "Adreno (TM) 420".
I have an open CL to suppress the Nexus 6 failure.

I think it makes more sense to suppress it than to rely on the stability of Nexus 6's driver, given it should be a very very similar driver to the buggy 5X driver.
Looks like Nexus 6 doesn't initialize locals like Nexus 5X does. This would mean it doesn't meet WebGL spec, I think, if you just suppress the failure.

Nexus 6 has OpenGL ES 3.1V@104.0 (GIT@I0ba6dce82d)
Nexus 5X has OpenGL ES 3.1 V@127.0 (GIT@I8366cd0437)

Looks like the bug wasn't present in the earlier version?
The problem is I don't know if there's a Nexus 6 build out there which does have the bug. So if we enable it, it could still cause crashes on some devices unless we spend a lot of effort to make sure it doesn't.

I think it's not worth that effort, and we should just take the conformance hit on Nexus 6 and similar devices. This bug was around for a very, very long time and AFAIK hasn't had significant impact.
I agree that avoiding crash is more important than conformance.

But, I also have another idea: we have Android L on Nexus 6 and Android M on Nexus 5X. I've seen that Anroid O doesn't crash. We don't yet know about Android N, I need to be at work to test it. Assuming N is also OK, we can disable initialization only on M, and monitor if we get crash reports in llvm to disable in more versions.

Your call.
IIRC I did not see the issue on N on 6P.

But, unfortunately, as aelias has pointed out before, Android version is not actually tied to the driver version - it's possible to have a phone with N and Adreno 4xx that still has the buggy driver.

If we want to put the time into it in the future we could enable it and watch out for crashes on canary/beta. For now I think I'd like to leave it as is. Thanks for the discussion!
Project Member

Comment 24 by bugdroid1@chromium.org, Oct 11 2017

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

commit 6d9af230633de615a840f63f1f575fcd2d2bd9d3
Author: Kai Ninomiya <kainino@chromium.org>
Date: Wed Oct 11 06:34:53 2017

Update expectations for uninitialized_local_global_variables

on Android/Adreno 420 (Nexus 6 bots).

Bug:  773294 ,  728271 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I8992f143d7320d8b69bab7d2573002179a82417a
Reviewed-on: https://chromium-review.googlesource.com/710076
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507911}
[modify] https://crrev.com/6d9af230633de615a840f63f1f575fcd2d2bd9d3/content/test/gpu/gpu_tests/webgl_conformance_expectations.py

Status: Fixed (was: Started)
Since there seem to have been no problems on the 5X bots, I'm going to close this again.

Sign in to add a comment