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

Use std::fmin instead of std::min in HcalTimeSlew::delay #12088

Merged
merged 1 commit into from Oct 30, 2015

Conversation

davidlt
Copy link
Contributor

@davidlt davidlt commented Oct 25, 2015

SIGSEGV (Segfault) was detected in HcalDeterministicFit::getLandauFrac
with ICC 16.0.0 20150815.

Segfault happens in RecoLocalCalo/HcalRecAlgos/src/HcalDeterministicFit.cc:42

42        sum= landauFrac[int(ceil(tStart+tsWidth))];

The function is called here in
RecoLocalCalo/HcalRecAlgos/interface/HcalDeterministicFit.h:112

112       getLandauFrac(-tsShift4,-tsShift4+tsWidth,i4);

But tsShift4 ends up being NAN:

112       getLandauFrac(-tsShift4,-tsShift4+tsWidth,i4);
(gdb) p tsShift4
$3 = nan(0x400000)
(gdb) p tsWidth
$4 = 25
(gdb) p i4
$5 = 0

HcalTimeSlew::delay use a log function and a negative argument in
passed.

(gdb) p fTimeSlew
$9 = HcalTimeSlew::InputPars
(gdb) p fTimeSlewBias
$10 = HcalTimeSlew::Medium
(gdb) p fpar0
$11 = 12.299899999999999
(gdb) p fpar1
$12 = -2.1914199999999999
(gdb) p fpar2 // par2
$13 = 0
(gdb) p inputCharge[4] // fC
$14 = (__gnu_cxx::__alloc_traits<std::allocator<double> >::value_type &)
@0x7fff38cf7e20: -0.08074517548084259

CalibCalorimetry/HcalAlgos/src/HcalTimeSlew.cc:23

return std::min(cap, par0 + par1*log(fC+par2));

Thus we end up with return std::min(6.0, NAN);

std::log has direct description of how it works if implementation
supports IEEE:

If the argument is negative, NaN is returned and FE_INVALID is
raised.

What std::min should return in case one of arguments is NAN is not directly
defined in C++ standard. Standard requires std::min to return the
first argumnet (6.0 in our case) if arguments are equivalent. All
comparison with NAN will return false. Becasue of this std::min
implementation with GCC and Clang will return the first argument. NAN
is returned in ICC case.

According to IEEE 754:

min(x,NaN) = min(NaN,x) = x

C++11 also introduced std::fmin from C standard, which provides a
direct instructions how NAN is handled:

.. between a NaN and a numeric value, the numeric value is chosen)

std::min is generic and does not direclty talk about IEEE
floating-points.

std::fmin works fine in correctly between GCC, Clang and ICC.

ICC issue will be reported for Intel for further discussions.

Ref: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47706

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

SIGSEGV (Segfault) was detected in `HcalDeterministicFit::getLandauFrac`
with ICC 16.0.0 20150815.

Segfault happens in RecoLocalCalo/HcalRecAlgos/src/HcalDeterministicFit.cc:42

    42        sum= landauFrac[int(ceil(tStart+tsWidth))];

The function is called here in
RecoLocalCalo/HcalRecAlgos/interface/HcalDeterministicFit.h:112

    112       getLandauFrac(-tsShift4,-tsShift4+tsWidth,i4);

But `tsShift4` ends up being **NAN**:

    112       getLandauFrac(-tsShift4,-tsShift4+tsWidth,i4);
    (gdb) p tsShift4
    $3 = nan(0x400000)
    (gdb) p tsWidth
    $4 = 25
    (gdb) p i4
    $5 = 0

`HcalTimeSlew::delay` use a `log` function and a negative argument in
passed.

    (gdb) p fTimeSlew
    $9 = HcalTimeSlew::InputPars
    (gdb) p fTimeSlewBias
    $10 = HcalTimeSlew::Medium
    (gdb) p fpar0
    $11 = 12.299899999999999
    (gdb) p fpar1
    $12 = -2.1914199999999999
    (gdb) p fpar2 // par2
    $13 = 0
    (gdb) p inputCharge[4] // fC
    $14 = (__gnu_cxx::__alloc_traits<std::allocator<double> >::value_type &)
    @0x7fff38cf7e20: -0.08074517548084259

CalibCalorimetry/HcalAlgos/src/HcalTimeSlew.cc:23

    return std::min(cap, par0 + par1*log(fC+par2));

Thus we end up with `return std::min(6.0, NAN);`

`std::log` has direct description of how it works if implementation
supports IEEE:

    If the argument is negative, NaN is returned and FE_INVALID is
    raised.

What `std::min` should return in case one of arguments is `NAN` is not directly
defined in C++ standard. Standard requires `std::min` to return the
first argumnet (6.0 in our case) if arguments are equivalent. All
comparison with `NAN` will return `false`. Becasue of this `std::min`
implementation with GCC and Clang will return the first argument. `NAN`
is returned in ICC case.

According to IEEE 754:

    min(x,NaN) = min(NaN,x) = x

C++11 also introduced `std::fmin` from C standard, which provides a
direct instructions how `NAN` is handled:

    .. between a NaN and a numeric value, the numeric value is chosen)

`std::min` is generic and does not direclty talk about IEEE
floating-points.

`std::fmin` works fine in correctly between GCC, Clang and ICC.

ICC issue will be reported for Intel for further discussions.

Ref: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47706

Signed-off-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @davidlt for CMSSW_8_0_X.

Use std::fmin instead of std::min in HcalTimeSlew::delay

It involves the following packages:

CalibCalorimetry/HcalAlgos

@cmsbuild, @mmusich, @diguida, @franzoni, @cerminar can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@davidlt
Copy link
Contributor Author

davidlt commented Oct 25, 2015

I can protect this with #ifdef __INTEL_COMPILER if desired.

Also found a good answer of SO: http://stackoverflow.com/a/16585438

Note, maybe the issue is not HcalTimeSlew::delay, but that the caller is providing unexpected values (negative numbers). Adding @slava77 @cvuosalo as caller is from RecoLocalCalo/HcalRecAlgos.

I was testing this on 5.1 workflow. Also tested on GCC by catching FP exceptions for std::log and negative numbers do come in at least once. E.g.,:

..
fC = -0.575, source = 3, bias = 1, par0 = 12.300, par1 = -2.191, par2 = 0.000
log_ret is nan
FE_INVALID raised! Numerical argument out of domain
..

But generic std::min returns the first argument (6.0), because all comparisons with NaN are false, thus arguments are considered "equivalent".

@davidlt
Copy link
Contributor Author

davidlt commented Oct 25, 2015

@smuzaffar this should make a good number of workflows running with ICC.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

davidlange6 added a commit that referenced this pull request Oct 30, 2015
Use std::fmin instead of std::min in HcalTimeSlew::delay
@davidlange6 davidlange6 merged commit c728165 into cms-sw:CMSSW_8_0_X Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants