Skip to content

Commit

Permalink
Use std::fmin instead of std::min in HcalTimeSlew::delay
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
David Abdurachmanov authored and David Abdurachmanov committed Oct 25, 2015
1 parent 1ee140f commit bf26bcd
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions CalibCalorimetry/HcalAlgos/src/HcalTimeSlew.cc
Expand Up @@ -20,10 +20,10 @@ double HcalTimeSlew::delay(double fC, ParaSource source, BiasSetting bias, doubl
return HcalTimeSlew::delay(fC, bias);
}
else if (source==InputPars) {
return std::min(cap, par0 + par1*log(fC+par2));
return std::fmin(cap, par0 + par1*log(fC+par2));
}
else if (source==Data || source==MC){
return std::min(cap,tspar0[source-1]+tspar1[source-1]*log(fC+tspar2[source-1]));
return std::fmin(cap,tspar0[source-1]+tspar1[source-1]*log(fC+tspar2[source-1]));
}
return 0;
}

0 comments on commit bf26bcd

Please sign in to comment.