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

Issue 332611 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Add safe integer support to base

Project Member Reported by jsc...@chromium.org, Jan 9 2014

Issue description

I'm planning on taking this in three phases:

1. Expand existing the conversion templates in safe_numerics.h to handle input floats, saturation, and track the type of conversion failures.

2. Add portable templates for checked and saturating integer math.

3. Optimize templates as needed using platform-specific mechanisms as appropriate.

 
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 17 2014

------------------------------------------------------------------------
r245418 | jschuh@chromium.org | 2014-01-17T03:32:40.342972Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/host/error_conversion.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/cloud_print/gcp20/prototype/printer_state.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/utility/extensions/unpacker.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/speech/google_streaming_remote_engine_unittest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/media_galleries/linux/mtp_read_file_worker.cc?r1=245418&r2=245417&pathrev=245418
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics/OWNERS?r1=245418&r2=245417&pathrev=245418
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/safe_numerics_unittest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/i18n/build_utf8_validator_tables.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/sandboxed_unpacker.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/win8/metro_driver/print_document_source.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/base/android/media_codec_bridge.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/sync/glue/session_model_associator.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/password_manager/password_manager_util_win.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/media/base/audio_bus.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/gpu/command_buffer/common/gles2_cmd_format.h?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/printing/print_destination_win.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/pepper/pepper_truetype_font_list_host.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/fileapi/blob_url_request_job_unittest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gypi?r1=245418&r2=245417&pathrev=245418
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics/safe_conversions.h?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/test_extension_dir.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/pepper/pepper_video_source_host.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/pepper/content_decryptor_delegate.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/convert_web_app.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/printing/image.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/mock_google_streaming_server.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/api/media_galleries/media_galleries_apitest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/printing/pdf_metafile_skia.cc?r1=245418&r2=245417&pathrev=245418
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics/safe_numerics_unittest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/net/crl_set_fetcher.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/media_galleries/win/mtp_device_operations_util.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/metrics/histogram_delta_serialization.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/components/policy/core/common/cloud/resource_cache.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/tools/imagediff/image_diff.cc?r1=245418&r2=245417&pathrev=245418
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics/safe_conversions_impl.h?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/child_process_sandbox_support_impl_linux.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/base.gyp?r1=245418&r2=245417&pathrev=245418
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/safe_numerics.h?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/vibration/vibration_message_filter.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/websockets/websocket_basic_stream.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/vibration/vibration_provider_android.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/base/clipboard/clipboard_win.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/components/plugins/renderer/webview_plugin.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/gpu/media/video_encode_accelerator_unittest.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/media/rtc_video_decoder.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/media_galleries/linux/snapshot_file_details.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/pepper/ppb_pdf_impl.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/password_manager/password_manager_metrics_util.cc?r1=245418&r2=245417&pathrev=245418
   A http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/websockets/websocket_channel_test.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/pepper/pepper_truetype_font_linux.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/net/websockets/websocket_channel.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/renderer_webkitplatformsupport_impl.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/win8/metro_driver/print_handler.cc?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/setup/install.cc?r1=245418&r2=245417&pathrev=245418
   D http://src.chromium.org/viewvc/chrome/trunk/src/base/safe_numerics_impl.h?r1=245418&r2=245417&pathrev=245418
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/snapshot/snapshot_aura.cc?r1=245418&r2=245417&pathrev=245418

Refactor base/safe_numerics.h

* Move into base/numerics subdirectory.
* Rename files for clarity.
* Add owners.
* Rename checked_numeric_cast to checked_cast.
* Fixup callsites and include paths.

BUG= 332611 
R=brettw@chromium.org, jam@chromium.org

Review URL: https://codereview.chromium.org/141113003
------------------------------------------------------------------------
Cc: zturner@chromium.org scottmg@chromium.org
I just looked at https://codereview.chromium.org/141583008/ to figure out why it was failing on vs2013.

The symptom is that it looks like numeric_limits<double>::max() is failing and returning NaN, which causes the test to fail. Since that function returns a constant, that's obviously unlikely.

Looking at the IntMaxOperations test, the NaN (-1#IND) is actually caused by an fp stack overflow. This seems to be happening due to earlier operations in the test, mostly in the <int64, float> one (even though the failure is later in the double test. The float branch is causing unbalanced fp stack ops and leaving crap on the stack.

I'm still investigating why that's happening.


I'm going to have to have a chat with whomever wrote those templates. :p

The imbalance is caused by converting floating point max()s to integral types in the TYPE_OVERFLOW test, e.g. in

  TEST_EXPECTED_VALIDITY(TYPE_OVERFLOW, checked_dst + SrcLimits::max());

Because the fp control word set to mask fp exceptions, the exception in ftol leaves the fp stack in an indeterminate state.

So, this is not a compiler problem.

FTR, two useful things I used to debug this. First, scattering calls to this function:

  void VerifyFpuStackEmpty() {
    unsigned int z[8];
    __asm {
      fldz
      fldz
      fldz
      fldz
      fldz
      fldz
      fldz
      fldz
      fstp dword ptr[z + 0x00]
      fstp dword ptr[z + 0x04]
      fstp dword ptr[z + 0x08]
      fstp dword ptr[z + 0x0c]
      fstp dword ptr[z + 0x10]
      fstp dword ptr[z + 0x14]
      fstp dword ptr[z + 0x18]
      fstp dword ptr[z + 0x1c]
    }

    for (unsigned int i = 0; i < 8; ++i) {
      CHECK_EQ(z[i], 0u);
    }
  }

tracked down where it was going unbalanced. After that mostly narrowed it down, unmasking FP exceptions at the beginning of the test pointed to where it was happening:

  _control87(0, _MCW_EM);

If you want to continue testing overflow, you need to clear the fp stack following known-exceptions. You can do that with the EMMS instruction, which is the _mm_empty() intrinsic.
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 28 2014

------------------------------------------------------------------------
r254005 | eroman@chromium.org | 2014-02-28T01:41:10.435815Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/numerics/safe_math_impl.h?r1=254005&r2=254004&pathrev=254005

Add required header for RangeConstraint definition.

BUG= 332611 

Review URL: https://codereview.chromium.org/183753003
------------------------------------------------------------------------
See https://code.google.com/p/chromium/issues/detail?id=348525#c20 ; we can probably remove the _mm_empty() spray post VS2013 Update2.

Comment 8 by jsc...@chromium.org, Jul 24 2014

Status: Fixed
Safe to say the initial implementation is done. I'll open a new bug for future performance improvements and refinements.

Sign in to add a comment