[io] Crash when removing leaf count array of non-primitive type

[hahnjo]

Check duplicate issues.

• Checked for duplicates

Description

In the context of schema evolution for CMS SoA data structures, we are seeing the following crash:

Fatal: kFALSE violated at line 1712 of `/path/to/root/io/io/src/TStreamerInfoReadBuffer.cxx'

This happens when removing a leaf count array of non-primitive type.

Reproducer

A reproducer is SoAEvolv.tar.gz, which is writing

struct Example {
   int fB;

   ClassDefNV(Example, 2)
};

struct SoAEvolv {
   int fElements = 0;
   int *fA = nullptr; //[fElements]
   Example *fE = nullptr; //[fElements]

   ClassDefNV(SoAEvolv, 2)
};

and then reading back into

struct SoAEvolv {
   int fElements = 0;
   int *fA = 0; //[fElements]

   ClassDefNV(SoAEvolv, 3)
};

ROOT version

6.36, but also latest master

Installation method

CMSSW, but also from source

Operating system

Linux

Additional context

FYI @Electricks94

[hahnjo]

The failing assertion is here:

root/io/io/src/TStreamerInfoReadBuffer.cxx

Lines 1709 to 1717 in c695dfb

	if (TStreamerInfo::kCache <= kase && kase < TStreamerInfo::kArtificial) {
	//T &cache_add = *(T*)b.PeekDataCacheArray();
	R__ASSERT(kFALSE); // cache_add);
	// thisVar->ReadBuffer(b,cache_addr,i,kase-TStreamerInfo::kCache,aElement,narr,eoffset)
	continue;
	}

It was introduced by the mega commit 11c16b6 almost 18 years ago...

[hahnjo]

Just removing the branch, like

diff --git a/io/io/src/TStreamerInfoReadBuffer.cxx b/io/io/src/TStreamerInfoReadBuffer.cxx
index 8135f9416fc..537f15eff08 100644
--- a/io/io/src/TStreamerInfoReadBuffer.cxx
+++ b/io/io/src/TStreamerInfoReadBuffer.cxx
@@ -1706,16 +1706,6 @@ Int_t TStreamerInfo::ReadBuffer(TBuffer &b, const T &arr,
          default: {
             int ans = -1;
 
-            if (TStreamerInfo::kCache <= kase && kase < TStreamerInfo::kArtificial) {
-
-               //T &cache_add = *(T*)b.PeekDataCacheArray();
-               R__ASSERT(kFALSE); // cache_add);
-
-               // thisVar->ReadBuffer(b,cache_addr,i,kase-TStreamerInfo::kCache,aElement,narr,eoffset)
-
-               continue;
-            }
-
             if (kase >= TStreamerInfo::kConv)
                ans = thisVar->ReadBufferConv(b,arr,compinfo[i],kase,aElement,narr,eoffset);
             if (ans==0) continue;

seems to work. And we are not exercising it anywhere in our tests, otherwise it would crash there. But I'm not sure if there is / was a reason for that protection in the first place...

[pcanal]

From the look of it the code says, if the kase involved caching and somehow is not explicit handled we should 'grab the cache address and pass it to the normal operation', however (most likely because it was not tested :)), it was commented out and asserted instead of being enabled untested.

So the questions are:
(a) In the use case that triggers the assert, does commenting the code leads to correct behavior?
(b) what is the value of kase and is it really not yet meant to be handled?

Even if (a), the range of cases covered by the if looks wide and we may want to be more surgical (i.e. explicitly handle that (or that type of) kase (value).

[hahnjo]

So the questions are:
(a) In the use case that triggers the assert, does commenting the code leads to correct behavior?

Yes. Which is not particularly surprising since we are removing a data member that should just be ignored. The other data member (fA) is still correctly read.

(b) what is the value of kase and is it really not yet meant to be handled?

According to the debugger after the crash, kase = 601 = kCache + kFloat.