Memory leak fix in SPIRVFunction::decodeBB

[bwlodarcz]

SPIRVEntry is created by function getEntry() but not deleted in a few frame ending branches. Fixes some ASan errors from #2233.

[asudarsa]

Hi @bwlodarcz

Can you please recheck this? I see that we eventually call into some code that returns an entry from a SPIRVFactory. Not sure if we can delete such pointers here.

Thanks

[MrSidims]

Can we move delete to Decoder d'tor eliminating all Entries from the map (or whatever container we use underneath getDecoder), or am I missing something?

[bwlodarcz]

First @asudarsa:

Can you please recheck this? I see that we eventually call into some code that returns an entry from a SPIRVFactory. Not sure if we can delete such pointers here.

Allocation of SPIRVEntry starts in template <typename T> SPIRVEntry *create() { return new T(); } on lib/SPIRV/libSPIRV/SPIRVEntry.cpp:62 - raw new/malloc.
As we can see in the reviewed function above we have Decoder.getEntry() call which points out to
SPIRVEntry *SPIRVDecoder::getEntry method located in lib/SPIRV/libSPIRV/SPIRVStream.cpp:239.
Overall we can see that it's some kind of factory method which initializes Entry.
During this initialization we have call to Entry->setModule(&M) where Entry points out to Module - but not the other way.

void SPIRVEntry::setModule(SPIRVModule *TheModule) {
  assert(TheModule && "Invalid module");
  if (TheModule == Module)
    return;
  assert(Module == NULL && "Cannot change owner of entry");
  Module = TheModule;
}

So right only the entry is aware of Module not vice versa.
And after that we are in our SPIRVFunction::decodeBB. From that point If the SPIRVEntry pointer is lost by some condition, before it get's assigned to SPIRVModule instance which happens later, we are unable to recover it by any means. That's why the delete/free is a necessity in such branches.
I rechecked and when the deletes are not in place than it leaks again.

Second @MrSidims:

Can we move delete to Decoder d'tor eliminating all Entries from the map (or whatever container we use underneath getDecoder), or am I missing something?

It seems that, in theory, in Decoder/Encoder iostream architecture SPIRVModule owns SPIRVEntries and it's responsible for the destruction - look on SPIRVModuleImpl destructor. In reality, SPIRVEntries are created outside of lifetime scope of SPIRVModule and eventually passed to it when we are coming through happy path. There are paths/branches where SPIRVEntry is created but doesn't fit (like in this PR) - in these cases SPIRVModule isn't an owner of the resource and it needs to be manually freed.
Changing an architecture of Decoder/Encoder is much more work and outside of scope of fixing direct ASan errors. I think that we, as community, should definitely think about such architectural change in future.

[asudarsa]

Same comment as above