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
__________________________
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Plplot-devel mailing list
Plplot-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/plplot-devel
