ConstantFoldingTest.FoldVectorCrossProduct fails under asan

[GoogleCodeExporter]

1. build with asan enabled (this is on linux):

$ GYP_DEFINES="clang=1 asan=1 lsan=1 target_arch=x64'" build/gyp_chromium
$ ninja -C out/Release/ angle_unittests

2. Run tests

$ out/Release/angle_unittests

Expected: Everything passes
Actual:

Retrying 1 test (retry #3)
Note: Google Test filter = ConstantFoldingTest.FoldVectorCrossProduct
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ConstantFoldingTest
[ RUN      ] ConstantFoldingTest.FoldVectorCrossProduct
../../third_party/angle/src/tests/compiler_tests/ConstantFolding_test.cpp:224: 
Failure
Value of: constantVectorFoundInAST(result)
  Actual: false
Expected: true
[  FAILED  ] ConstantFoldingTest.FoldVectorCrossProduct (3 ms)
[----------] 1 test from ConstantFoldingTest (3 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (3 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:

Original issue reported on code.google.com by thakis@chromium.org on 15 Jun 2015 at 10:22

[GoogleCodeExporter]

Can you investigate this Arun? There may be an uninitialized read or something 
going on here.

Original comment by jmad...@chromium.org on 15 Jun 2015 at 10:30

• Changed state: Assigned

[GoogleCodeExporter]

Nico, do you have repro steps?

Original comment by jmad...@chromium.org on 15 Jun 2015 at 10:30

[GoogleCodeExporter]

See comment 0?

Original comment by thakis@chromium.org on 15 Jun 2015 at 10:35

[GoogleCodeExporter]

The most likely explanation for me is that in asan mode we build with -O1 
instead of -O2 
(https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&l=
2270) and since this test is doing things with floats it does so in an 
optimizer-dependent way where float stuff ends up being imprecise. That's 
admittedly pretty hand-wavy, but that's the direction I'd investigate this in.

Original comment by thakis@chromium.org on 15 Jun 2015 at 10:39

[GoogleCodeExporter]

Oh yes, sorry, I missed that.

Original comment by jmad...@chromium.org on 15 Jun 2015 at 10:40

[GoogleCodeExporter]

Sure, I will check this and update.

Original comment by apat...@nvidia.com on 16 Jun 2015 at 3:37

[GoogleCodeExporter]

I could reproduce it with my linux build after enabling asan. Problem doesn't 
look to be specific to the test but looks like it is exposed by this test.

I tried debugging it and looks like input values itself are not getting passed 
correctly. There could be some problem representing constructors like vectors 
(vec2/vec3/vec4), matrices, etc.

I just declared one vector like "vec3 v = vec3(1.0f, 1.0f, 1.0f)" in test 
shader and tried to inspect values of constant union array created by vec3 
constructor. All the values were MAX_FLOAT instead of 1.0f. Same I observed for 
vec2 as well. These incorrect values are getting passed to the folding function 
and the test fails because we get incorrect results.

Original comment by apat...@nvidia.com on 16 Jun 2015 at 8:15

[GoogleCodeExporter]

On this particular type of build, there looks to be a problems with flex/bison 
or generated parser. It is failing to parse float constants with suffix 'f'. If 
I just remove suffix from the test, it starts passing. Debugging it more to see 
what exact problem we have in parsing float constants with suffix.

Original comment by apat...@nvidia.com on 16 Jun 2015 at 10:16

[GoogleCodeExporter]

Use of atof_clamp here looks to be a problem: 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/angle/src
/compiler/translator/glslang_lex.cpp&l=3342
If I just use atof instead, it starts working.

atof_clamp[1] just uses pp::numeric_lex_float[2] and looks like some other 
tools like Dr. Memory also reported memory errors with this function earlier on 
windows[3]. So, this memory error was suppressed earlier for Dr. memory[4] and 
then un-suppressed because it was a real STL bug[5].

[1] 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/angle/src
/compiler/translator/util.cpp&cl=GROK&ct=xref_jump_to_def&l=15&gsn=atof_clamp
[2] 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/angle/src
/compiler/preprocessor/numeric_lex.h&cl=GROK&l=49&gsn=atof_clamp&rcl=1434439730
[3] https://code.google.com/p/chromium/issues/detail?id=349416
[4] https://codereview.chromium.org/183023009
[5] https://codereview.chromium.org/196863003

Original comment by apat...@nvidia.com on 16 Jun 2015 at 12:38

[GoogleCodeExporter]

If removing the "f" suffix from floats in shaders is enough to work around this 
atof_clamp bug, I'd suggest we do that (in both existing and upcoming tests).

Original comment by oetu...@nvidia.com on 16 Jun 2015 at 12:52

[GoogleCodeExporter]

[4] and [5] are about Microsoft's standard library which isn't used on Linux. 
So I'm not sure that's related to this failure on linux here.

Original comment by thakis@chromium.org on 16 Jun 2015 at 2:34

[GoogleCodeExporter]

And it sounds like there's a bug in the float parsing code which this test 
seems to observe. Isn't it better to fix that bug instead of changing the test 
so that it's no longer observed?

I'm not super familiar with flex, but glslang.l contains this:

int floatsuffix_check(TParseContext* context)
{
    struct yyguts_t* yyg = (struct yyguts_t*) context->getScanner();

    if (context->getShaderVersion() < 300)
    {
        context->error(*yylloc, "Floating-point suffix unsupported prior to GLSL ES 3.00", yytext);
        context->recover();
        return 0;
    }

    if (!atof_clamp(yytext, &(yylval->lex.f)))
        yyextra->warning(*yylloc, "Float overflow", yytext, "");

    return(FLOATCONSTANT);
}

{D}+{E}[fF]           { return floatsuffix_check(context); }
{D}+"."{D}*({E})?[fF] { return floatsuffix_check(context); }
"."{D}+({E})?[fF]     { return floatsuffix_check(context); }

Doesn't this mean that yytext will include the trailing f, i.e. yytext will be 
(say) "1.0f"?

pp::numeric_lex_float just uses a istringstream to do the actual conversion, 
can that handle the trailing f? It doesn't look like it:

Nicos-MacBook-Pro-3:v8 thakis$ cat test.cc
#include <sstream>
#include <stdio.h>

template<typename FloatType>
bool numeric_lex_float(const std::string &str, FloatType *value)
{
    std::istringstream stream(str);
    // Force "C" locale so that decimal character is always '.', and
    // not dependent on the current locale.
    stream.imbue(std::locale::classic());

    stream >> (*value);
    return !stream.fail();
}

int main() {
  float f;
  bool b = numeric_lex_float("1.0f", &f);
  fprintf(stderr, "%d %f\n", b, f);
  b = numeric_lex_float("1.0", &f);
  fprintf(stderr, "%d %f\n", b, f);
}
Nicos-MacBook-Pro-3:v8 thakis$ clang++ test.cc && ./a.out
0 0.000000
1 1.000000

But then I'd expect this test to fail in more places, so maybe I just missed 
where the f is stripped, or lex doesn't work like I think it does.

Original comment by thakis@chromium.org on 16 Jun 2015 at 2:52

[GoogleCodeExporter]

I think your analysis is correct Nico. I'm not sure why we don't error out in 
other non-ASAN tests with the 'f' suffix, but we should add some tests and fix 
the parsing bug. If you feel like taking a look Arun feel free, otherwise 
someone can look at fixing this for ES3.

Original comment by jmad...@chromium.org on 16 Jun 2015 at 4:22

[GoogleCodeExporter]

Yeah, I will try to fix if there is something wrong.

Nico,
As per some documentation, character reading is stopped by numeric extractor 
once invalid character is reached, so may be that f should be ignored. This 
could also vary across platforms as you are getting different results with your 
sample.

Here is the reference:
http://www.cplusplus.com/reference/istream/istream/operator%3E%3E/ 
<-"...Then (if good), it calls num_get::get (using the stream's selected 
locale) to perform both the extraction and the parsing operations..."

http://www.cplusplus.com/reference/locale/num_get/get/
<- "...The function stops reading characters from the sequence as soon as one 
character cannot be part of a valid numerical expression (or end is reached)..."

Anyways, I will look more into it tomorrow.

Original comment by apat...@nvidia.com on 16 Jun 2015 at 6:01

[GoogleCodeExporter]

Issue chromium:501013 has been merged into this issue.

Original comment by thakis@chromium.org on 16 Jun 2015 at 10:53

[GoogleCodeExporter]

Arun's reading of the reference looks correct to me. Also, we know that parsing 
"1.0f" yields the expected result at least on some platforms. So this looks 
more like a platform bug rather than a bug in the ANGLE code (one that we 
probably should work around though). I also still think that this shouldn't be 
tested by the constant folding test for cross(). If we want to test this, it's 
easy enough to write a separate test case.

Original comment by oetu...@nvidia.com on 17 Jun 2015 at 9:12

[GoogleCodeExporter]

Debugged this further, it doesn't look to be a difference between asan and 
non-asan chromium builds but the difference between using libstdc++ and using 
libc++.

"1.0f" gets successfully parsed to "1.0" with libstdc++ but fails with libc++. 
Chromium uses libc++ only for instrumental builds like asan build and that's 
the reason we see this issue with only asan builds.

Sample in comment #12 works for me even on non-asan builds when I use libstdc++ 
but fails when I use libc++ using "-stdlib=libc++" flag. So, this most probably 
is libc++ bugs as the standard looks to be saying that invalid chars should 
just be ignored. Or libc++ just preferred to do it that way. I asked it on one 
similar libc++ bug : https://llvm.org/bugs/show_bug.cgi?id=17782#c13

No matter whether this behavior is intentionally like this in libc++ or it is a 
bug, I think best way to work around this would be to just remove tailing 
suffixes like 'f' before our parses passes it to atof_clamp/numeric_lex_float.

Original comment by apat...@nvidia.com on 17 Jun 2015 at 10:56

[GoogleCodeExporter]

> Chromium uses libc++ only for instrumental builds

Chromium uses libc++ on iOS and Android for production builds.

Original comment by thakis@chromium.org on 17 Jun 2015 at 2:47

[GoogleCodeExporter]

Stumbled on this problem while running unittests on Mac. Put up a CL here: 
https://chromium-review.googlesource.com/#/c/278210/

Original comment by cwal...@google.com on 17 Jun 2015 at 6:05

[GoogleCodeExporter]

This has been broken for quite a while now. It'd be good to get some fix in 
soon.

Original comment by thakis@chromium.org on 18 Jun 2015 at 8:39

[GoogleCodeExporter]

Ping. Who owns this?

Original comment by thakis@chromium.org on 19 Jun 2015 at 8:26

[GoogleCodeExporter]

Currently in review: https://chromium-review.googlesource.com/#/c/278210/

Corentin, any update?

Original comment by jmad...@chromium.org on 19 Jun 2015 at 8:29

• Changed state: Started

[GoogleCodeExporter]

Also, Nico, apologies for the slow responses but our entire team is in MTV this 
week and we have a pretty busy schedule. This fix should be in soon.

Original comment by jmad...@chromium.org on 19 Jun 2015 at 8:30

[GoogleCodeExporter]

The following revision refers to this bug:
  https://chromium.googlesource.com/angle/angle/+/e2adce1ba29e389dbb6ec50d71558f187b134452

commit e2adce1ba29e389dbb6ec50d71558f187b134452
Author: Corentin Wallez <cwallez@chromium.org>
Date: Fri Jun 19 21:06:32 2015

Workaround for some STLs not parsing floats with a suffix

BUG=angleproject:1046
BUG=angleproject:891

Change-Id: I01c3f024142e573385a5f40d721171ad752ffd19
Reviewed-on: https://chromium-review.googlesource.com/278210
Reviewed-by: Brandon Jones <bajones@chromium.org>
Tested-by: Corentin Wallez <cwallez@chromium.org>

[modify] 
http://crrev.com/e2adce1ba29e389dbb6ec50d71558f187b134452/src/compiler/translato
r/glslang.l
[modify] 
http://crrev.com/e2adce1ba29e389dbb6ec50d71558f187b134452/src/compiler/translato
r/glslang_lex.cpp
[modify] 
http://crrev.com/e2adce1ba29e389dbb6ec50d71558f187b134452/src/compiler/translato
r/util.cpp
[modify] 
http://crrev.com/e2adce1ba29e389dbb6ec50d71558f187b134452/src/compiler/translato
r/util.h

Original comment by bugdroid1@chromium.org on 19 Jun 2015 at 10:35

[GoogleCodeExporter]

Original comment by cwal...@chromium.org on 19 Jun 2015 at 10:40

• Changed state: Fixed

[GoogleCodeExporter]

cwallez: Are you or somenoe else rolling this into Chromium?

Original comment by h...@chromium.org on 22 Jun 2015 at 7:42

[GoogleCodeExporter]

Geoff will roll it in right now.

Original comment by jmad...@chromium.org on 22 Jun 2015 at 7:46