Memory leak in MSI

[Martin Konrad]

MSI in Base 3.15 has a memory leak:

$ cd src/ioc/dbtemplate/test/O.linux-x86_64-debug
$ valgrind --leak-check=full --track-origins=yes ./msi-copy -I. -I.. -S ../t2-substitution.txt
[...]
==3718== 4,096 bytes in 1 blocks are definitely lost in loss record 67 of 69
==3718== at 0x4839775: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3718== by 0x10C682: catMacroReplacements (msi.c:1120)
==3718== by 0x10C11C: substituteGetReplacements (msi.c:984)
==3718== by 0x10A92E: main (msi.c:196)
[...]

[Martin Konrad (info-martin-konrad)]

Merge proposal: https://code.launchpad.net/~info-martin-konrad/epics-base/+git/epics-base/+merge/361500

[Ralph Lange (ralph-lange)]

This hits the negative end of my personal relevance scale: a memory leak in a short running tool only used at build time.

At the same time a nice way to raise the overall software quality without noticeably improving anything. So: why not.

[Martin Konrad (info-martin-konrad)]

Fixing this issue came as a byproduct of rewriting catMacroReplacements() to fix https://bugs.launchpad.net/epics-base/+bug/1810946. The latter was actually preventing us from moving to 3.15.6.

[Ralph Lange (ralph-lange)]

Sorry for the harsh tone.

I was surprised to see a bug report and a branch and a merge proposal for this issue, which is certainly nice to have been fixed but IMHO doesn't deserve that level of publicity.

[mdavidsaver (mdavidsaver)]

On 1/9/19 7:03 AM, Ralph Lange wrote:
> Sorry for the harsh tone.
>
> I was surprised to see a bug report and a branch and a merge proposal
> for this issue, which is certainly nice to have been fixed but IMHO
> doesn't deserve that level of publicity.

Seriously ?!?

If we want to encourage people to contribute then we need to give them
something in return. We can't pay them, nor they us. Martin is big
on formal process* and probably also wants something he can show his
management to justify the time spent.

* imo too big, but that's a different conversation

[Martin Konrad (info-martin-konrad)]

On one hand I agree that this leak is benign and that there's not too much value in having a ticket for it. On the other hand, I would consider every leak a bug. And it wouldn't be the first time a leak led me to a piece of code that had other issues as well.

But, actually, the rationale behind filing this bug was to make the Valgrind output available to reviewers rather than just mentioning that my MR also fixes a leak somewhere (just wanted to spare reviewers the time of firing up Valgrind themselves). After submitting I realized that I also could have attached this information as a comment to the merge proposal (I'm still learning Launchpad). Sorry about the noise...

BTW: Did anyone have a chance to look at the MP?

[mdavidsaver (mdavidsaver)]

> Seriously ?!?

well oops... I hadn't meant this to go to the list. My mistake.
Ralph and Martin you have my apologies

[mdavidsaver (mdavidsaver)]

> Sorry about the noise...

The gist of the discussion I was having with Ralph is that I don't see this as noise!
Please don't let this discourage you.

I see real long term value in cleanup. If it leave the code base in better shape for subsequent work. If it also fixes some minor issues, then all the better.

As I reminded Ralph, but didn't stupidly CC to everyone, one of my first contributions to Base was converting some K&R style C to the "new" ANSI style. An entirely non-functional change.

> BTW: Did anyone have a chance to look at the MP?

I intend to do so today. I can see right away that one suggestion will be using std::auto_ptr as a local instead of a local raw pointer in eg. substituteOpen().