Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1096113003:
One more ASAN fix. (Closed)
|
Created:
5 years, 8 months ago by Stephen White Modified:
5 years, 8 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionOne more ASAN fix.
Don't fail the GL validation check if the GL context fails to create.
TBR=robertphillips@google.com
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/409fd66a5afcef5f165f7ccec7c3473add231752
Patch Set 1 #
Messages
Total messages: 19 (4 generated)
Rob: PTAL. Thanks!
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096113003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the interest of green bots, I'm going to land this TBR.
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096113003/1
On 2015/04/21 16:05:34, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1096113003/1 Well ... it seems like a context creation we expect to succeed actually failing might be worth an assert.
On 2015/04/21 16:06:38, robertphillips wrote: > On 2015/04/21 16:05:34, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1096113003/1 > > Well ... it seems like a context creation we expect to succeed actually failing > might be worth an assert. This seems to be causing ASAN, UBSAN and TSAN bots to fail. Should they be succeeding to create GL contexts? If not, why are they compiling with SK_GPU_ENABLED?
robertphillips@google.com changed reviewers: + borenet@google.com, mtklein@google.com
I don't know about that but I've added some people who may.
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/409fd66a5afcef5f165f7ccec7c3473add231752
Message was sent while issue was closed.
Looks like this CL caused some new bots to turn green: https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... ... but it looks like they're just skipping the tests because the GL context doesn't get created? I think we need this assert to make it clear that these bots aren't actually testing anything.
Message was sent while issue was closed.
On 2015/04/21 17:08:26, borenet wrote: > Looks like this CL caused some new bots to turn green: > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > ... but it looks like they're just skipping the tests because the GL context > doesn't get created? I think we need this assert to make it clear that these > bots aren't actually testing anything. I think the value of this test is that, when we *do* manage to create a GL context, it calls validate() on the interface (line 39). Brian can correct me if I'm wrong. On non-ASAN bots, removing that call means that if we don't manage to create a GL context, the test won't fail. (I do think we'll have bigger problems than this one test, however). If the ASAN bots can't create GL contexts, there's no way we can validate them anyway. If ASAN bots are never going to be able to create GL contexts, one possibility would be to compile them without SK_SUPPORT_GPU, or perhaps just not to run the GPU tests on them, so they don't fail.
Message was sent while issue was closed.
> If ASAN bots are never going to be able to create GL contexts, one possibility > would be to compile them without SK_SUPPORT_GPU, > or perhaps just not to run the GPU tests on them, so they don't fail. That's what we've been doing. These new bots have GPUs. We just don't have them working yet.
Message was sent while issue was closed.
On 2015/04/21 17:56:44, Stephen White wrote: > On 2015/04/21 17:08:26, borenet wrote: > > Looks like this CL caused some new bots to turn green: > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > ... but it looks like they're just skipping the tests because the GL context > > doesn't get created? I think we need this assert to make it clear that these > > bots aren't actually testing anything. > > I think the value of this test is that, when we *do* manage to create a GL > context, it calls validate() on the interface > (line 39). Brian can correct me if I'm wrong. > > On non-ASAN bots, removing that call means that if we don't manage to create a > GL context, the test won't fail. > (I do think we'll have bigger problems than this one test, however). > > If the ASAN bots can't create GL contexts, there's no way we can validate them > anyway. > > If ASAN bots are never going to be able to create GL contexts, one possibility > would be to compile them without SK_SUPPORT_GPU, > or perhaps just not to run the GPU tests on them, so they don't fail. These particular bots just aren't set up correctly yet, and this assertion was keeping them red. I got confused when they suddenly turned green. Anyway, mtklein@ just submitted a change to make DM exit if a particular config can't be run. That will send these bots back red until they're fixed.
Message was sent while issue was closed.
On 2015/04/21 18:02:38, borenet wrote: > On 2015/04/21 17:56:44, Stephen White wrote: > > On 2015/04/21 17:08:26, borenet wrote: > > > Looks like this CL caused some new bots to turn green: > > > > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > ... but it looks like they're just skipping the tests because the GL context > > > doesn't get created? I think we need this assert to make it clear that > these > > > bots aren't actually testing anything. > > > > I think the value of this test is that, when we *do* manage to create a GL > > context, it calls validate() on the interface > > (line 39). Brian can correct me if I'm wrong. > > > > On non-ASAN bots, removing that call means that if we don't manage to create a > > GL context, the test won't fail. > > (I do think we'll have bigger problems than this one test, however). > > > > If the ASAN bots can't create GL contexts, there's no way we can validate them > > anyway. > > > > If ASAN bots are never going to be able to create GL contexts, one possibility > > would be to compile them without SK_SUPPORT_GPU, > > or perhaps just not to run the GPU tests on them, so they don't fail. > > These particular bots just aren't set up correctly yet, and this assertion was > keeping them red. I got confused when they suddenly turned green. Anyway, > mtklein@ just submitted a change to make DM exit if a particular config can't be > run. That will send these bots back red until they're fixed. OK, in that case I'll revert this change.
Message was sent while issue was closed.
On 2015/04/21 18:02:38, borenet wrote: > On 2015/04/21 17:56:44, Stephen White wrote: > > On 2015/04/21 17:08:26, borenet wrote: > > > Looks like this CL caused some new bots to turn green: > > > > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-G... > > > > > > ... but it looks like they're just skipping the tests because the GL context > > > doesn't get created? I think we need this assert to make it clear that > these > > > bots aren't actually testing anything. > > > > I think the value of this test is that, when we *do* manage to create a GL > > context, it calls validate() on the interface > > (line 39). Brian can correct me if I'm wrong. > > > > On non-ASAN bots, removing that call means that if we don't manage to create a > > GL context, the test won't fail. > > (I do think we'll have bigger problems than this one test, however). > > > > If the ASAN bots can't create GL contexts, there's no way we can validate them > > anyway. > > > > If ASAN bots are never going to be able to create GL contexts, one possibility > > would be to compile them without SK_SUPPORT_GPU, > > or perhaps just not to run the GPU tests on them, so they don't fail. > > These particular bots just aren't set up correctly yet, and this assertion was > keeping them red. I got confused when they suddenly turned green. Anyway, > mtklein@ just submitted a change to make DM exit if a particular config can't be > run. That will send these bots back red until they're fixed. Yes, don't worry, I'll fix things and get those bots broken right away! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
