Modify

Opened 8 years ago

Closed 5 years ago

#3869 closed Bugs (fixed)

Unconditional call to memset in value_initialized()

Reported by: Aleksey Gurtovoy 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 8 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 8 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 8 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 8 years ago by Niels Dekker

Attachment: config_compiler.patch added

Adds BOOST_VALUE_INITIALIZATION_NEEDS_MEMSET to trunk/boost/config/compiler

comment:1 Changed 8 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 8 years ago by John Maddock

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 8 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 8 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 8 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 8 years ago by Niels Dekker

Attachment: value_init_workaround.patch added

Made memset call conditional, based on BOOST_NO_COMPLETE_VALUE_INITIALIZATION

comment:5 Changed 8 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: fixed
Status: newclosed

Closed, as suggested by Vicente (viboes).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Niels Dekker.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.