Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac OSX: parseNumber tests failing for out-of-bound numbers on clang #45

Closed
umar456 opened this issue Dec 10, 2015 · 3 comments
Closed

Comments

@umar456
Copy link
Contributor

umar456 commented Dec 10, 2015

The following tests are failing for parseNumber using clang++ Apple LLVM version 6.0 (clang-600.0.57) (based on LLVM 3.5svn).:

[  FAILED  ] AssemblyContextParseNarrowUnsignedIntegers.Sample
[  FAILED  ] AssemblyContextParseWideUnsignedIntegers.Sample
[  FAILED  ] AssemblyContextParseFloat.Sample
[  FAILED  ] AssemblyContextParseDouble.Sample

This is because clang does not fail for large numbers(i.e. 1e400) when converting text to float using istringstream.

#include <iostream>
#include <iomanip>
#include <sstream>
#include <limits>
using namespace std;

int main() {
  istringstream text_stream("1e400");
  double val = 0;

  text_stream >> std::setbase(0);
  text_stream >> val;

  cout << text_stream.fail() << endl;
  cout << "val: " << val << endl;
}

Output:

clang++ -std=c++11 foo.cpp -o clang_foo && echo clang && ./clang_foo && g++-4.8 -std=c++11 foo.cpp -o gcc_foo && echo gcc && ./gcc_foo
clang
0
val: inf
gcc
1
val: 1.79769e+308
@dneto0
Copy link
Collaborator

dneto0 commented Dec 10, 2015

The reliance on the failure from istringstream is used to check for out-of-bounds arguments to things like OpConstant %type , where %type is a known type.
For example if %type is an unsigned 32-bit int, then the istringstream::fail() is used to see if the value is out of bounds for that type.
In short, this is behaviour we do want to rely on, so that the assembler can properly check its inputs.

This looks like a bug in the Apple C++ library.
Behaviour is as specified in http://en.cppreference.com/w/cpp/io/basic_istream/operator_gtgt
In particular "f extraction fails, zero is written to value and failbit is set. If extraction results in the value too large or too small to fit in value, std::numeric_limits::max() or std::numeric_limits::min() is written and failbit flag is set."

Well, we've already worked around a GNU libc++ bug. We seem to have an Apple one.

Thanks for reporting this.

@dneto0 dneto0 changed the title parseNumber tests failing for out-of-bound numbers on clang Mac OSX: parseNumber tests failing for out-of-bound numbers on clang Feb 3, 2016
@dneto0
Copy link
Collaborator

dneto0 commented Feb 3, 2016

I investigated two test failures which occur on Mac OSX using Apple's Clang.

There are two failing essential cases:

  1. parsing "-0" is expected to succeed with value 0 even for unsigned target types.
  2. parsing a too-large floating point value in regular notation (e.g. 1.0e400) is expected to result in an error, but it does not.

dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Feb 6, 2016
Works around a difference between Xcode's std::istringstream
and that of other platforms.

Addresses part of KhronosGroup#45
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Feb 6, 2016
Works around a difference between Xcode's std::istringstream
and that of other platforms.

Addresses part of KhronosGroup#45
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Feb 8, 2016
Also checks some hex literal cases.

This addresses part of
KhronosGroup#45
by removing the parseNumber case for "-0" on unsigned
integers.  We don't care about that platform difference
at the level of std::istringstream, since we reject it
at a higher parsing level.
dneto0 added a commit to dneto0/SPIRV-Tools that referenced this issue Feb 8, 2016
Also checks some hex literal cases.

This addresses part of
KhronosGroup#45
by removing the parseNumber case for "-0" on unsigned
integers.  We don't care about that platform difference
at the level of std::istringstream, since we reject it
at a higher parsing level.
dneto0 added a commit that referenced this issue Feb 8, 2016
Also checks some hex literal cases.

This addresses part of
#45
by removing the parseNumber case for "-0" on unsigned
integers.  We don't care about that platform difference
at the level of std::istringstream, since we reject it
at a higher parsing level.
@dneto0
Copy link
Collaborator

dneto0 commented Feb 13, 2016

Fixed in master with 6bad02c

@dneto0 dneto0 closed this as completed Feb 13, 2016
dgkoch pushed a commit to dgkoch/SPIRV-Tools that referenced this issue Nov 21, 2018
- SPV_EXT_shader_stencil_export
- SPV_AMD_shader_image_load_store_lod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants