Avoid LIKELY/UNLIKELY

I may have been too quick in merging this because this MR triggers a compilation warning in TCKPrescaleEmulator.cpp, seen in the gcc12-opt platforms of all the above nightlies - I don't understand why. @clemenci or @graven could you have a look please?

In file included from ../Kernel/LHCbAlgs/src/TCKPrescaleEmulator.h:17,
                 from ../Kernel/LHCbAlgs/src/TCKPrescaleEmulator.cpp:11:
In function 'std::ostream& LHCb::operator<<(std::ostream&, const HltDecReports&)',
    inlined from 'MsgStream& operator<<(MsgStream&, const T&) [with T = LHCb::HltDecReports]' at /workspace/build/Gaudi/InstallArea/x86_64_v2-centos7-gcc12-opt/include/GaudiKernel/MsgStream.h:304:20,
    inlined from 'virtual StatusCode TCKPrescaleEmulator::execute()' at ../Kernel/LHCbAlgs/src/TCKPrescaleEmulator.cpp:108:51:
../Event/HltEvent/include/Event/HltDecReports.h:124:107: warning: 'this' pointer is null [-Wnonnull]
  124 |     friend std::ostream& operator<<( std::ostream& str, const HltDecReports& obj ) { return obj.fillStream( str ); }
      |                                                                                             ~~~~~~~~~~~~~~^~~~~~~
In function 'std::ostream& LHCb::operator<<(std::ostream&, const HltDecReports&)',
    inlined from 'MsgStream& operator<<(MsgStream&, const T&) [with T = LHCb::HltDecReports]' at /workspace/build/Gaudi/InstallArea/x86_64_v2-centos7-gcc12-opt/include/GaudiKernel/MsgStream.h:304:20,
    inlined from 'virtual StatusCode TCKPrescaleEmulator::execute()' at ../Kernel/LHCbAlgs/src/TCKPrescaleEmulator.cpp:148:51:
../Event/HltEvent/include/Event/HltDecReports.h:124:107: warning: 'this' pointer is null [-Wnonnull]
  124 |     friend std::ostream& operator<<( std::ostream& str, const HltDecReports& obj ) { return obj.fillStream( str ); }
      |                                                                                             ~~~~~~~~~~~~~~^~~~~~~

Actually this looks like a bug: there is a check on null pointer, the printout is done if the pointer is null and then the null pointer is used! @graven?

if ( !decreports ) {
    if ( msgLevel( MSG::VERBOSE ) ) verbose() << *decreports << endmsg;
    HltDecReports* reports = new HltDecReports();
    reports->setConfiguredTCK( decreports->configuredTCK() );
    reports->setTaskID( decreports->taskID() );

There's a real bug in the code, but it looks like the use of UNLIKELY hid it from the compiler (and gcc12 got better at static analysis).

The code

if ( !decreports ) {
  if ( msgLevel( MSG::VERBOSE ) ) verbose() << *decreports << endmsg;

is clearly wrong because we dereference something we just made sure is nullptr.

If you look at the else branch, it looks clear that it should have been if ( decreports ) { and not if ( !decreports ) {.

Now my question to @graven is how this code could possible work.

Indeed, we came to the same conclusion, sorry @clemenci for wasting your time. The bug fix is easy to do and should be in a separate MR that we then back-port to the legacy branches, but @graven should first comment on the implications. @rmatev FYI

@graven @rmatev ping

@graven ping. Could you please spare a moment to check what the implications of this bug would have been on the Run2 and Run1 HLT, if any?

I had a quick look. The bug was introduced at the end of 2015 with 9aa21a1d

This code was never used in HLT. I suspect this algorithm has not been used in offline productions, too. User jobs might have been affected, but that's obviously hard to know.

@cofitzpa as the author might give more insight. The use case for the algorithm is described in https://twiki.cern.ch/twiki/bin/view/LHCb/EmulatingPrescales

Admin message

Avoid LIKELY/UNLIKELY

Merge request reports

Activity