Harden merging of VLAs#15368
Conversation
| if (((TLeaf*)br->GetListOfLeaves()->First())->GetLeafCount() != nullptr) { | ||
| int maximum = ((TLeaf*)br->GetListOfLeaves()->First())->GetLeafCount()->GetMaximum(); | ||
| auto* sizeLeaf = ((TLeaf*)br->GetListOfLeaves()->First())->GetLeafCount(); | ||
| // GetMaximum() can be unreliable (may return 0 or a stale value). |
There was a problem hiding this comment.
This comment is based on?
It may be 0 when all VLAs are empty, right? What would be the stale value?
There was a problem hiding this comment.
I am also suspicious of that statement and I couldn't find any clue of it being correct, however the check itself is only potentially useless, but not wrong. Given the file is actually malformed*, I would keep it. Maybe we can move the validation behind an environment variable?
- Tested with:
void test() {
TFile *f = TFile::Open("AO2D.root");
for (auto *key : *f->GetListOfKeys()) {
auto *dir = dynamic_cast<TDirectoryFile*>(f->Get(key->GetName()));
if (!dir) continue;
for (auto *k2 : *dir->GetListOfKeys()) {
auto *t = dynamic_cast<TTree*>(dir->Get(k2->GetName()));
if (!t) continue;
long long expected = t->GetEntries();
for (auto *b : *t->GetListOfBranches()) {
auto *br = (TBranch*)b;
if (br->GetEntries() != expected) {
printf("MISMATCH %s/%s branch %s: tree=%lld branch=%lld\n",
key->GetName(), k2->GetName(), br->GetName(), expected, br->GetEntries());
}
}
}
}| if (maximum == 0) { | ||
| maximum = 1; // all entries are empty arrays, but we still need a valid buffer | ||
| } |
There was a problem hiding this comment.
I don't know how much CPU the scanning of the whole branch takes. Maybe not important.
Wouldn't these 3 lines be enough. Do we really need to check the validity of GetMaximum(). Shall we ask the ROOT team about it?
| if (exitCode > 0) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Is this additional bail-out needed?
Protect for the case all the VLAs are empty. Validate number of entries if fast cloning is used.
|
Updated to only check for 0 (which is for sure UB) and to validate the number of entries after a fast clone. |
| // detect VLA | ||
| if (((TLeaf*)br->GetListOfLeaves()->First())->GetLeafCount() != nullptr) { | ||
| int maximum = ((TLeaf*)br->GetListOfLeaves()->First())->GetLeafCount()->GetMaximum(); | ||
| maximum = std::max(maximum, 1); |
There was a problem hiding this comment.
| maximum = std::max(maximum, 1); | |
| maximum = std::max(maximum, 1); // Can be 0 if all VLAs are empty but we need a valid buffer below |
|
This is fine with me. @sawenzel @catalinristea As the merger is also used in MC productions and reco, could you also check if you are OK with it. Many thanks! |
2 participants
Protect for the case all the VLAs are empty.
Validate number of entries if fast cloning is used.