[SERVER-48076] implicit conversion from 'long long' to 'double' in oplog_stone_parameters_gen.cpp Created: 11/May/20  Updated: 29/Oct/23  Resolved: 22/Sep/21

Status: Closed
Project: Core Server
Component/s: Build, Security
Affects Version/s: None
Fix Version/s: 5.1.0-rc0

Type: Bug Priority: Major - P3
Reporter: Jiang Li Assignee: Reo Kimura (Inactive)
Resolution: Fixed Votes: 1
Labels: neweng
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by SERVER-59233 Fix or suppress warnings when buildin... Closed
Related
related to SERVER-48079 implicit conversion from 'long long' ... Closed
related to SERVER-60199 Coverity analysis defect 120812: Redu... Closed
related to SERVER-57633 Remove disable warnings as error for ... Closed
is related to SERVER-46071 Server parameters with AtomicWord<int... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Steps To Reproduce:

Compile with clang-10

Sprint: Execution Team 2021-09-20, Execution Team 2021-10-04
Participants:

 Description   

src/mongo/bson/bsonelement.h:1021:22: error: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-
Werror,-Wimplicit-int-float-conversion]
if ((d > std::numeric_limits<T>::max()) || (d < std::numeric_limits<T>::lowest())) {
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/mongo/idl/server_parameter_with_storage.h:251:43: note: in instantiation of function template specialization 'mongo::BSONElement::tryCoerce<long long>' re
quested here
if (auto status = newValueElement.tryCoerce(&newValue); !status.isOK()) {
^
src/mongo/idl/server_parameter_with_storage.h:192:5: note: in instantiation of member function 'mongo::IDLServerParameterWithStorage<mongo::ServerParameterTyp
e::kStartupOnly, long long>::set' requested here
IDLServerParameterWithStorage(StringData name, T& storage)
^
src/mongo/idl/server_parameter_with_storage.h:326:16: note: in instantiation of member function 'mongo::IDLServerParameterWithStorage<mongo::ServerParameterTy
pe::kStartupOnly, long long>::IDLServerParameterWithStorage' requested here
return new IDLServerParameterWithStorage<paramType, T>(name, storage);
^
build/opt/mongo/db/storage/wiredtiger/oplog_stone_parameters_gen.cpp:32:21: note: in instantiation of function template specialization 'mongo::makeIDLServerPa
rameterWithStorage<mongo::ServerParameterType::kStartupOnly, long long>' requested here
auto* ret = makeIDLServerParameterWithStorage<ServerParameterType::kStartupOnly>("maxOplogTruncationPointsAfterStartup", gMaxOplogStonesAfterStartup);



 Comments   
Comment by Vivian Ge (Inactive) [ 06/Oct/21 ]

Updating the fixversion since branching activities occurred yesterday. This ticket will be in rc0 when it’s been triggered. For more active release information, please keep an eye on #server-release. Thank you!

Comment by Githook User [ 22/Sep/21 ]

Author:

{'name': 'Reo Kimura', 'email': 'reo.kimura@mongodb.com', 'username': 'rkimura21'}

Message: SERVER-48076 Account for rounding of 64 bit type max in conversion to…
Branch: master
https://github.com/mongodb/mongo/commit/b16a1efdc1e16dcd75151641d2d6dd9f2619ef8b

Comment by Benety Goh [ 19/Aug/21 ]

The compiler warning should no longer be emitted in clang 10 after this commit.

We are leaving this ticket open to follow up on an improvement to BSONElement::tryCoerce() suggested during the code review process.

Comment by Githook User [ 18/Aug/21 ]

Author:

{'name': 'Benety Goh', 'email': 'benety@mongodb.com', 'username': 'benety'}

Message: SERVER-57633 SERVER-48076 fix implicit long long to double conversion in BSONElement::tryCoerce()
Branch: master
https://github.com/mongodb/mongo/commit/ced65a176cab202d5abf57488f777189f27d7e41

Comment by Geert Bosch [ 14/Sep/20 ]

Thanks for reaching out, we'll be fixing this.

Comment by Andrew Shuvalov [ 12/Sep/20 ]

I had a busy week and had no time to follow up. I agree with comments above and I closed the pull request for now. I also found some logic to check the boundaries in src/mongo/util/summation.h, but indeed the code in represent_as.h looks good and should be invoked from every location including summation.h.

Comment by Geert Bosch [ 09/Sep/20 ]

We should correctly check for these boundary conditions, as otherwise it's too easy to get into undefined behavior territory. The way I implemented overflow checking for other languages is to check that the double precision number is strictly less than the max of the integer typ e + 1. We can use ldexp to create that constant. This is also done in represent_as.h. It might be good to reuse that code.

Comment by Sara Golemon [ 08/Sep/20 ]

Looking at the error message, it's not so much a question of boundaries as resolution.

An IEEE double is 64bits wide, but only 53 bits of that are available to store a precise value, so clang-10 is rightly letting us know that we'll potentially lose fidelity when we try to convert a value larger than +/- 2^52.

Ignoring the warning is probably sufficient for our use cases, chiefly that coercing double to int64_t should be rare, and doing so with a runtime value 2^52 < x < 2^63 moreso.  So I wouldn't say no to just merging this PR, but it should probably come with some runtime detection to toss out debug level log statements warning of the loss of precision, if for no better reason that to get a handle on how much of a problem it is in practice.

And on this specific case (as well as many other IDL config types), as the value is unlikely to be meaningful beyond 4 billion, it might be worthwhile to choose a storage type of std::uint32_t and avoid the entire issue.

Comment by Andrew Shuvalov [ 08/Sep/20 ]

I think the existing code is trying to check for 'double' boundaries before the conversion to 'long long' because the 'coerce(&val)' method is not super smart and it will try to cast 'double' to 'long long' without checking for proper double boundaries. It looks correct to me like it is, no actual code change is needed, just proper respect to the compiler.

Comment by Andrew Shuvalov [ 08/Sep/20 ]

Hi Geert, created a pull request as you described: https://github.com/mongodb/mongo/pull/1375

In this particular case the fix is trivial, as described in PR comments. There are some other spots where more work is needed, and tests should be added. 

Comment by Geert Bosch [ 08/Sep/20 ]

It is not clear to me why this would involve a conversion to floating-point, rather than use the constructor here.

Comment by Geert Bosch [ 08/Sep/20 ]

Hi androxylo@gmail.com, generally the way to propose a bug fix is to send a GitHub pull request, see https://github.com/mongodb/mongo/wiki for more info.

Comment by Andrew Shuvalov [ 08/Sep/20 ]

I have a fix for this error reproduced with clang version 10 in my local repository. If you explain me how to propose code changes (never did it before), I will send you MR.

Comment by Carl Champain (Inactive) [ 11/May/20 ]

Hi liangjie.mailbox@qq.com,

Thanks for the report. I'm passing this ticket along to the appropriate team for additional investigation. Updates will be posted as they happen.

Kind regards,
Carl
 

Generated at Thu Feb 08 05:16:04 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.