Modify

Opened 7 years ago

Closed 5 years ago

#3869 closed Bugs (fixed)

Unconditional call to memset in value_initialized()

Reported by: agurtovoy Owned by: niels_dekker
Milestone: Boost 1.42.0 Component: utility
Version: Boost 1.41.0 Severity: Problem
Keywords: Cc: agurtovoy@…

Description

The call is there to workaround bugs in a few specific compilers, but is applied unconditionally, cannot be optimized away, and thus significantly limits applicability of the component in a library context.

See http://old.nabble.com/boost::mpl::for_each-and-value_initialized-td25246848.html for the full discussion.

Attachments (4)

config_compiler.patch (3.4 KB) - added by niels_dekker 7 years ago.
Adds BOOST_VALUE_INITIALIZATION_NEEDS_MEMSET to trunk/boost/config/compiler
no_complete_value_initialization.patch (10.9 KB) - added by niels_dekker 7 years ago.
New proposed patch, adding BOOST_NO_COMPLETE_VALUE_INITIALIZATION. Replaces the old config_compiler.patch (Update: removed Boost.Test dependency, as suggested by John Maddock.)
value_init_workaround.patch (1.6 KB) - added by niels_dekker 7 years ago.
Made memset call conditional, based on BOOST_NO_COMPLETE_VALUE_INITIALIZATION
value_init_workaround_test.cpp (3.3 KB) - added by niels_dekker 7 years ago.

Download all attachments as: .zip

Change History (14)

Changed 7 years ago by niels_dekker

Adds BOOST_VALUE_INITIALIZATION_NEEDS_MEMSET to trunk/boost/config/compiler

comment:1 Changed 7 years ago by niels_dekker

Fernando (the author of value_init) mailed me that it's okay to him if I add an #ifdef to utility/value_init.hpp:

  #ifdef BOOST_VALUE_INITIALIZATION_NEEDS_MEMSET
       std::memset(&x, 0, sizeof(x));
  #endif

But therefor, this macro needs to be defined in <boost/config/compiler/...>, for the relevant compiler versions. Hereby I have attached my proposed additions to the trunk version of boost/config/compiler: config_compiler.patch Please check if it's okay.

comment:2 Changed 7 years ago by johnmaddock

The patch is OK as far as it goes, but we should have documentation and tests for each new macro, please see: http://www.boost.org/doc/libs/1_41_0/libs/config/doc/html/boost_config/guidelines_for_boost_authors.html#boost_config.guidelines_for_boost_authors.adding_new_defect_macros

Thanks, John.

comment:3 Changed 7 years ago by niels_dekker

Thanks, John. I read that there's a BOOST_NO_SOMETHING naming convention. So now I'm considering to name the macro BOOST_NO_PROPER_VALUE_INITIALIZATION_FOR_ALL_TYPES. (Instead of BOOST_VALUE_INITIALIZATION_NEEDS_MEMSET.) So that the unit test should show that compilers that have this macro defined do indeed do an improper value-initialization, for at least one type.

I'll have a closer look later...

comment:4 Changed 7 years ago by niels_dekker

Hereby I have just added a new patch for boost/config (to be applied to the trunk): https://svn.boost.org/trac/boost/attachment/ticket/3869/no_complete_value_initialization.patch It adds a new macro, BOOST_NO_COMPLETE_VALUE_INITIALIZATION, and includes documentation and tests. Please tell me what you think.

Changed 7 years ago by niels_dekker

New proposed patch, adding BOOST_NO_COMPLETE_VALUE_INITIALIZATION. Replaces the old config_compiler.patch (Update: removed Boost.Test dependency, as suggested by John Maddock.)

Changed 7 years ago by niels_dekker

Made memset call conditional, based on BOOST_NO_COMPLETE_VALUE_INITIALIZATION

comment:5 Changed 7 years ago by niels_dekker

The value_init_workaround.patch I just attached should resolve this ticket. It assumes that the defect macro BOOST_NO_COMPLETE_VALUE_INITIALIZATION is defined for those compilers that need it (ticket #4080). Note that it adds an extra macro, BOOST_DETAIL_VALUE_INIT_WORKAROUND (defined as either 0 or 1), which controls whether the memset workaround is applied. This macro is merely intended as an implementation detail: I would like to have an extra test which toggles the value of the macro, in order to check if the workaround is both effective and necessary.

comment:6 Changed 7 years ago by niels_dekker

(In [62307]) Made memset call in value_init conditional, see #3869. Updated the section "compiler issues" of its documentation.

Changed 7 years ago by niels_dekker

comment:7 Changed 7 years ago by niels_dekker

(In [63014]) Added value_init_workaround_test, reviewed by Fernando Cacciola, see #3869

comment:8 Changed 7 years ago by niels_dekker

(In [63638]) Merged value_init fixes (extra tests + documentation) from trunk, see #3472, #3869.

comment:9 Changed 5 years ago by viboes

Is there something else to be added? If not, could you close the issue?

comment:10 Changed 5 years ago by niels_dekker

  • Resolution set to fixed
  • Status changed from new to closed

Closed, as suggested by Vicente (viboes).

Add Comment

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain niels_dekker.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.