Out-of-bounds memory access in random color choice

[nscottnichols]

In process_sigmaKin_function.inc on lines 64 and 175, channelIdC == -1 when channel enhancement is disabled (channelId == 0). This causes an out-of-bounds memory access on lines 72, 184, and 194 (mgOnGpu::icolamp[channelIdC][icolC]). Channel enhancement is always on in the full application (correct me if I'm wrong here) so this isn't a problem, but in the standalone, we hardcode channelId=0 on line 575 in check_sa.cc. The proposed fix is to simply hardcode channelId=1, in check_sa.cc. Or, we could protect the code with an if (channelIdC > 0), at the cost of the if statement.

The out-of-bounds memory access causes hangs when compiled for Nvidia devices in the SYCL code (no issues when compiled for devices from other vendors).

@valassi

[valassi]

Thanks @nscottnichols ! I will have a look (but not immediately sorry)

[valassi]

Very interesting anyway, this is somehow also related to the #607 I just discussed with @zeniheisser

I would still keep channelId=0 in

madgraph4gpu/epochX/cudacpp/CODEGEN/PLUGIN/CUDACPP_SA_OUTPUT/madgraph/iolibs/template_files/gpu/check_sa.cc

Line 575 in 93bee87

constexpr unsigned int channelId = 0; // TEMPORARY? disable multi-channel in check.exe and gcheck.exe #466

Instead it would probably be better to disable the random choice of color (hence prevent the access to icolamp[0]) if channelId=0, which is very much related to #607.

This is actually a question for @oliviermattelaer : do I understand correctly that in a way the concept of "leading color" (one of which must be chosen in the random choice of color) ONLY makes sense if a specific channel has been defined? I think we discussed that and I had understood that this is the case (because icolamp defines which colors are leading colors for a given channelId), but then I might be wrong.

(En passant, we still need to fix the way coloramps.h is generated from coloramps.inc maybe - or maybe not, I need to read my notes again, but we had closed #574 as wontfix and this was related)

[valassi]

Hi @nscottnichols I have just merged MR #709. This upgrades cudacpp codegen to @oliviermattelaer 's latest select_color version. This fixes #710, which sounds very similar to this #611.

Can you try to upgrade your sycl codegen and check if this issue is fixed there please? Thanks Andrea

[valassi]

Hi @nscottnichols I am reviewing some old issues and I think that this one sounds very similar to one that I just fixed, See commit 6ac6cf9 in upcoming MR #706, this is needed to fix #783.

Precisely as you report, there is one line channelIdC=channelId-1 that produces -1 if channelId==0. This happens within the random color choice. The fix I just added is that, when channelId==0, then the random color choice should be skipped completely (@oliviermattelaer should still confirm). This avoids creating an index -1 (which by the way I just turned into an unsigned int). It avoids an out of bound memory access for you, and a floating point exception for me.

I would close this if you agree. Thanks again (and sorry for not paying attention earlier on, it would have helped als to debug the FPEs).

[valassi]

Instead it would probably be better to disable the random choice of color (hence prevent the access to icolamp[0]) if channelId=0, which is very much related to #607.

This is actually a question for @oliviermattelaer : do I understand correctly that in a way the concept of "leading color" (one of which must be chosen in the random choice of color) ONLY makes sense if a specific channel has been defined? I think we discussed that and I had understood that this is the case (because icolamp defines which colors are leading colors for a given channelId), but then I might be wrong.

I had made the comments above in March, and indeed this is what I have just done today in MR #706 to fix #783.

Note also yesterday's patch #796 which is also related to channelId==0 and the lack of multichannel support.

@nscottnichols, I close this as done (at least for cudacpp, not sycl).

Copying also @roiser @oliviermattelaer