Re: [Plplot-devel] First proof-of-concept versions of PLplot "memory allocation" and "exception handling" topics

[Alan W. I. <ir...@be...>]

On 2017-07-17 11:31+0100 Phil Rosenberg wrote:

> Hi Alan
> Please find attached a new patch series.
>
> I should have fixed the segfault issue. Note I have done some rebasing
> so you will probably need to create new branches. I think there is a
> memory leak somewhere. I will look into this.
>
> But you may have something you can play with to test a bit better now.
> I have added specific comments below.
>

Hi Phil:

Thanks for your reply with substantial updates to both branches.  To
make this process more convenient for me _next time_ would you be willing to
store your memory allocation patches in one tarball (e.g., your first
6 patches this time) and your exception handling patches (e.g., your
remaining 6 patches with the first 6 excluded) in another tarball?
This time I have already split the 12 patches into two separate
locations, see below, but that is a lot of error-prone cutting and
pasting of filenames so two tarballs next time would be much better.

[...]
> Yes, the memory management is independent of exception handling, but
> exception handling requires memory management to avoid memory leaks.

OK. See how I handled these patches below.

@Others:

This is how I applied Phil's patches.

git checkout master
# New branch
git checkout -b memory_allocation2
# Specific location where I stored the first 6 of Phil's series that had
# to do with memory allocation.  This cat trick is something I just
# discovered, and it makes it much easier to apply a series of patches
# like these.
software@raven> cat ~irwin/Phil.Rosenberg/20170717/memory_allocation/* |git am
Applying: Added functions that will record memory allocations in the PLStream
Applying: Part way through plmalloc implementation
Applying: Removed plinitializememorylist and plfreeall calls
Applying: Fix issue with growing memorylist
Applying: Added missing ;
Applying: Added Memory allocation function declarations to plplotP.h

git checkout master
# New branch
git checkout -b exception_handler2
# Apply patches in memory_allocation2 since this branch depends on
# that one being applied first.
git rebase memory_allocation2
# Specific location, etc.
software@raven> cat ~irwin/Phil.Rosenberg/20170717/exception_handler/* |git am
Applying: Added Exception handling macros/functions/variables
Applying: Add some test code for exception handling
Applying: Fixed PLENDTRY/PLTHROW bug
Applying: Remove plexit calls from advancejumpbuffer and plinitialisememorylist
Applying: Added some comments around PLTRY, PLCATCH and PLENDTRY
Applying: Free memory from jumpbuffer stack in plend1

@Phil:

Again all seems clean with applying your commits for both these topic branches.

> This [good valgrind results for memory_allocation branch] is not surprising. I Don't think the plmalloc or plrealloc
> functions are actually used anywhere yet :-D

I thought that might be the case, but these good valgrind results for
all C examples are a good benchmark, and, in fact, that good benchmark
has immediately paid off as a comparison for a bad result for thise
second version of the memory allocation topic, see below.

>
>>
>> Furthermore, I like what you have done with plmalloc, plrealloc, and
>> plfree where if inside those routines malloc, realloc, or free
>> respectively fail, plexit is called with appropriate error message.
>> Assuming, then you want to replace all our current direct calls to
>> malloc, realloc and free with these pl* variants, this solves a major
>> problem we have which was all the wide range of different treatments
>> of how malloc, realloc and free errors were checked, i.e., in the best
>> cases plexit is called, but more usually no error checking is done at
>> all!
>
> Not quite. Any pointers which we store in the plstream must use the
> original malloc, otherwise they will be freed as soon as the API call
> exits.
> There are two options here.
> 1) Rely on developers to choose malloc or plmalloc as needed. But when
> I looked through the code, it wasn't always obvious because there are
> some places where memory is allocated in one bit of code, then the
> assignment of the pointer in the plstream occurs somewhere else. I
> think this is quite messy.
> 2) Have plfreeall check that the memory allocations do not correspond
> to the pointers in plstream. This only requires that developers modify
> plfreeall if they add a new heap pointer to plstream. it may be that
> this is easy to miss as it would be done infrequently, but it would
> probably very quickly generate a segfault or valgrind would spot it
> easily.
>
> My preference is I think for 2, but I welcome comments.

I suggest instead that you create a list (which will, of course, need
to be maintained) of plstream heap pointers where a pointer to that
list is stored in plstream.  The advantage of this location for the
list is it can then be used not only in plfreeall, but also in
plmalloc, plcalloc, etc.  to do the right thing in those functions
whenever you are dealing with a plstream heap pointer.  Which, in
turn, would allow us to always use plmalloc, plcalloc, etc.,
everywhere in our C library.  Which is much less error prone (other
than the maintenance required for keeping that list updated, as you
point out above) since developers then know they must always use the
pl* versions of memory management calls while letting all those
different pl* memory allocation and freeing functions deal with
whether the heap pointers are in plstream or not.

With regard to testing the above memory_allocation2, I repeated the
same test as before (used the -DVALGRIND_ALL_TESTS=ON cmake option,
built the test_c_psc target, and checked for any imperfect valgrind
reports).  Examples 00 through 07 were fine, but example 08 segfaulted.
I have attached the associated valgrind report to help you to debug
this issue.

That finishes the memory allocation topic, and I now want to move
on to the exception handling topic.

Since this topic branch depends on the memory allocation branch, and
there is currently a showstopper segfault in that branch, I did
no tests of this exception handling topic branch other than
the one above that showed these commits applied cleanly.

However, I do have some further responses to your remarks about
this branch below.

[...]
> Hopefully some or all of this [segfault in the prior version of this branch] is fixed now. I would welcome another
> valgrind summary.

Current problem is in memory_allocation branch, see above.

>> * There is currently no ability for PLTHROW to propagate an error message.
>>   Is that a fundamental limitation of setjmp/longjmp based C exception
>>   handling or could that feature be easily implemented?  If the latter,
>>   then that gives us the option to use PLTHROW wherever errors occur
>>   rather than always calling plexit which then prints the error
>>   message before it uses PLTHROW.  So for example in plmalloc,
>>   plcalloc, plrealloc, and plfree you could use PLTHROW directly
>>   rather than calling plexit. However, if implementation of handling
>>   error messages for PLTHROW is difficult/impossible, then I would be
>>   content to call plexit everywhere so that there would only be one
>>   PLTHROW (within plexit after printing out the error message) in our
>>   code as now on this topic branch.
>
> We could throw an error code, but I don't think an error message.
> However, I think it would be better to put an error message/code in
> the plstream.

Agreed.  I personally don't like error codes that much.  It takes
me back to the 70's when I first had to deal with IBM "reason
codes"
<https://www.ibm.com/support/knowledgecenter/en/SS2L7A_5.1.0/doc/ccv-reason-codes.html>.
such as the dreaded OOC5 and OOC6.

> This would need to be on the heap rather than the stack
> otherwise it would be lost in the stack unwinding. I think we should
> maintain a function like plerror that would put the error code/message
> onto the stream, but maybe renamed to be called something like plthrow

Go for it.

[out of order]
>> * I suggest you protect your exception handling code (but not the
>>   separate topic branch code concerning pl* versions of malloc,
>>   calloc, realloc, and free) with an PL_IMPLEMENT_EXCEPTION_HANDLING
>>   macro that is controlled (using #cmakedefine in plplot_config.h.in)
>>   by a cmake option in cmake/modules/plplot.cmake of the same name
>>   which defaults to OFF and which is self-documented as highly
>>   experimental. Once you do this, then it makes it possible to merge
>>   this topic quite rapidly to master (to facilitate further
>>   collaborative development of our exception handling system) since
>>   the user would have to go out of their way to set
>>   -DPL_IMPLEMENT_EXCEPTION_HANDLING=ON once they found
>>   PL_IMPLEMENT_EXCEPTION_HANDLING in CMakeCache.txt. And that file
>>   includes the self-documentation of each option so it should be
>>   obvious to such users that turning that option ON is highly
>>   experimental.
>
> Would you be happy to add this variable to the build system for me?
> Your cmake knowledge is  much better than mine. Or do I just use that
> macro and adding of -DPL_IMPLEMENT_EXCEPTION_HANDLING=ON to the
> command line automagically generate the equivalent #define.

This is the way that we implement most of our optional C
macros so you should be able to deal with this yourself
either following the above description or following the pattern of any
of the other macros set in plplot_config.h.in.  But if you
prefer not to do that, than you could leave the CMake part of
this to me, and as a temporary measure place

#define PL_IMPLEMENT_EXCEPTION_HANDLING

in plplot.h and then do the more painstaking part of this task
which is to sprinkle

#ifdef PL_IMPLEMENT_EXCEPTION_HANDLING

and appropriate #else and #endif lines throughout wherever your
changes have added to the existing code.  The goal here, of course, is
the net result of your changes is exactly equivalent to the memory
allocation branch if PL_IMPLEMENT_EXCEPTION_HANDLING is not #defined,
and I will follow up by creating a CMake option to insure this macro
is not #defined by default unless you decide you want to do that
yourself following the above instructions.

>
>>
>> * Can you explain why you have implemented all the PLTRY blocks
>>   in src/plvpor.c (with presumably the idea in mind to use PLTRY
>>   in a lot more places?)  Wouldn't it be better to use just one
>>   PLTRY block?  For example, could you get away with just that one PLTRY
>>   block in the library initialization code? If so, the corresponding
>>   PLCATCH block there could write out a message saying you reached
>>   that catch block (to confirm that the whole exception handling system is
>>   fundamentally working within our C library), and then exit (for now)
>>   in just that one place rather than doing nothing about whatever
>>   PLplot library error was detected.
>
> I didn't check the API, but I thought that the naming convention was
> that every function beginning c_ is part of the public API. Is this
> not correct? EVERY public API function would need a PLTRY block. I
> only chose this file because it contains plenv, which was one of the
> first API functions called by x00c

You are correct, everything in c_* is in the public API.  However, I still
don't see the need for PLTRY and PLCATCH blocks for each of our public
API's.

I assume this gets into the question of how you are going to propagate
errors outside the library.  I cannot remember the details of your
previous discussion of that topic, but I was left with the impression
you were spoiled for choice.  Furthermore, if you were planning to
propagate the error outside the PLplot library (to, e.g., x00c.c) for
each of the currently empty PLCATCH blocks in src/plvpor.c, I claim
could instead be done at one central location in the plplot library
(i.e., in a PLCATCH block in the library initialization code that also
shuts down most/all of the library from that one central location).  I
think it is a no-brainer to handle library errors centrally like that
if possible since it is less intrusive and therefore much less editing
work and much less error prone, but if I am missing something (e.g.,
that is not possible with any of the propagation methods you had in
mind), then the ideal way to demonstrate that is to include an example
in your proof-of-concept of propagating a PLplot library error to
x00c.c.

Alan
__________________________
Alan W. Irwin

Astronomical research affiliation with Department of Physics and Astronomy,
University of Victoria (astrowww.phys.uvic.ca).

Programming affiliations with the FreeEOS equation-of-state
implementation for stellar interiors (freeeos.sf.net); the Time
Ephemerides project (timeephem.sf.net); PLplot scientific plotting
software package (plplot.sf.net); the libLASi project
(unifont.org/lasi); the Loads of Linux Links project (loll.sf.net);
and the Linux Brochure Project (lbproject.sf.net).
__________________________

Linux-powered Science
__________________________