While testing a few ideas with pointers, I bumped into a behavior not very clear to me.
During the development of one of the modules I'll be using in a project of mine, there came a point where I decided I'll simply assign the address of a pointer variable passed as a paramater as the value of one of an object's fields.
While writing the code for the said object's destructor, I decided to test what would happen to the variable, whose address I've assigned to an object's field, after I've applied Dispose() to the latter.
Here's a sample of what I was trying to do:
program BankTest;
var BankCredit : PCredit; BankSample : PBank;
begin New(BankCredit); BankCredit^ := 1000;
New(BankSample, BankCredit); BankSample^.Destroy; writeln(BankSample^); Dispose(BankCredit); end.
TBank.Create has this:
constructor TBank.Create(amount : PCredit); begin New(Self.AccountBalance); Self.AccountBalance := amount; end;
TBank.Destroy has this:
destructor TBank.Destroy; begin Dispose(Self.AccountBalance); end;
I get no errors or warnings (compiled with -O2 and -Wall), but when I try to run the resulting example, I get a `Segmentation fault' where writeln(BankSample^) is.
Here are my questions:
1) Is this correct behavior according to the standards? I know that doing two New()'s on the same pointer is a recipe for disaster, but it is unclear to me whether the same applies to this scenario.
2) If this is correct (i.e., I'm doing something I shouldn't be doing), then why doesn't GPC catch it? If this is already planned, then I guess I should just be more careful; but if it isn't, would it be hard to implement this?
I apologize for the long message, but all of the Pascal resources I have aren't very clear about this particular situation. Any clues as to where I should look (and/or help in general) would be greatly appreciated.
TIA.
Neil Santos wrote:
While testing a few ideas with pointers, I bumped into a behavior not very clear to me.
During the development of one of the modules I'll be using in a project of mine, there came a point where I decided I'll simply assign the address of a pointer variable passed as a paramater as the value of one of an object's fields.
While writing the code for the said object's destructor, I decided to test what would happen to the variable, whose address I've assigned to an object's field, after I've applied Dispose() to the latter.
I get no errors or warnings (compiled with -O2 and -Wall), but when I try to run the resulting example, I get a `Segmentation fault' where writeln(BankSample^) is.
Here are my questions:
1) Is this correct behavior according to the standards? I know that doing two New()'s on the same pointer is a recipe for disaster,
No, it isn't. Don't know where you got this from.
but it is unclear to me whether the same applies to this scenario.
No, this scenario *is* a problem. ;-)
According to a standard it's an error that a processor is permitted to leave undetected. It's in the same category as using undefined values etc.
2) If this is correct (i.e., I'm doing something I shouldn't be doing), then why doesn't GPC catch it?
Tracking such things is possible, but with *very* high effort. (Both in terms of compiler code and runtime inefficiency.) If we allow combination with other languages, it becomes even harder if not impossible.
Some people suggest heuristics, such as setting undefined variables to certain "unlikely" values and runtime-testing for these, but they can only catch some of the problems, and are not completely reliable.
You can try linking the "Electric fence" library to catch some kinds of memory management errors. The same caveat as in the previous paragraph apply, but at least it's no big effort to do.
I apologize for the long message, but all of the Pascal resources I have aren't very clear about this particular situation. Any clues as to where I should look (and/or help in general) would be greatly appreciated.
I suppose that's because in most compilers (including GPC, BP, ...) it's just undefined behaviour.
Frank
On 17:44 12/06/04, Frank Heckenbach wrote:
1) Is this correct behavior according to the standards? I know that doing two New()'s on the same pointer is a recipe for disaster,No, it isn't. Don't know where you got this from.
`Turbo Pascal 6: The Complete Reference', by Stephen O'Brien, published by Borland and Osborne. Said two New()'s on the same pointer will allocate two chunks of memory of sizeof(variable) on the heap, and two corresponding Dispose()'s will not get rid of the two allocations.
but it is unclear to me whether the same applies to this scenario.No, this scenario *is* a problem. ;-)
According to a standard it's an error that a processor is permitted to leave undetected. It's in the same category as using undefined values etc.
Ahh. Then I guess I should just be careful.
Some people suggest heuristics, such as setting undefined variables to certain "unlikely" values and runtime-testing for these, but they can only catch some of the problems, and are not completely reliable.
No, this wouldn't help. It would just lull people into a false sense of security; better just tell people to be wary of their code (which they should always be, in any case).
You can try linking the "Electric fence" library to catch some kinds of memory management errors. The same caveat as in the previous paragraph apply, but at least it's no big effort to do.
Thanks for the tip; will try it as soon as appropriate.
I suppose that's because in most compilers (including GPC, BP, ...) it's just undefined behaviour.
That would certainly account for the discrepancy. :)
Neil Santos wrote:
1) Is this correct behavior according to the standards? I know that doing two New()'s on the same pointer is a recipe for disaster,No, it isn't. Don't know where you got this from.
`Turbo Pascal 6: The Complete Reference', by Stephen O'Brien, published by Borland and Osborne. Said two New()'s on the same pointer will allocate two chunks of memory of sizeof(variable) on the heap, and two corresponding Dispose()'s will not get rid of the two allocations.
Yes, the two `Dispose's on the same value are a disaster if you will, two `New's are just a memory leak, often harmless.
Frank
Frank Heckenbach wrote:
Yes, the two `Dispose's on the same value are a disaster
For that very reason, it is a good idea to call Dispose wrapped:
procedure MyDispose( var thePtr: Pointer); begin if thePtr <> nil then begin Dispose( thePtr); thePtr:= nil end end;
Of course, if the pointer-value (itself) has been copied, it can still be diposed twice. To prevent that, you need a well designed program structure in which it is clear who owns the pointer.
The above procedure makes memory management easier also.
begin thePtr:= nil;
....
MyDispose( thePtr) end;
If .... is a complicated sequence of conditional statements, it may be that New( thePtr) isn't called. Still, memory management will be correct.
Finally, as a special bonus, you get a runtime error when you dereference the MyDisposed pointer, as it has a NIL value.
Regards,
Adriaan van Os
Adriaan van Os wrote:
Frank Heckenbach wrote:
Yes, the two `Dispose's on the same value are a disaster
For that very reason, it is a good idea to call Dispose wrapped:
procedure MyDispose( var thePtr: Pointer); begin if thePtr <> nil then begin Dispose( thePtr); thePtr:= nil end end;
Of course, if the pointer-value (itself) has been copied, it can still be diposed twice. To prevent that, you need a well designed program structure in which it is clear who owns the pointer.
IMHO, in such a design it's usually also clear when to dispose it, so the wrapper might not be needed ...
The above procedure makes memory management easier also.
begin thePtr:= nil;
....
MyDispose( thePtr) end;
GPC allows disposing of nil except in standard modes.
Frank
At 23:51 +0000 12/6/04, Neil Santos wrote:
constructor TBank.Create(amount : PCredit); beginNew(Self.AccountBalance); Self.AccountBalance := amount; end;
This pair of lines shows a deep lack of understanding of what a pointer is and how pointers work.
The first line allocates some memory for you and puts the address of the memory into the pointer variable Self.AccountBalance.
The second line copies the address from the pointer variable amount into the pointer variable Self.AccountBalance. Thus the address to the area of memory you allocated is now lost entirely, guaranteeing a memory leak. Memory leaks are not fatal (unless you leak enough), but they almost always indicate incorrect programming (the only real exception being memory you allocate once and never release and rely on the system to clean up for you when your application exits - and since you only allocate it once you have a finite amount of "leaked" memory so your program wont eventually die).
The next main problem with the routine is more conceptual - who owns the memory and who will be responsible for destroying it later? Any time you pass in a pointer and a copy of the pointer is kept, you are setting yourself up for problems unless you are very clear about the ownership of the pointer.
You program would be fine if you added a comment like:
(* The TBank.Create constructor's parameter is a pointer which must be a validly New'ed pointer and which becomes the property of TBank and which will be Dispose'd when the TBank object is Destroyed *)
and then you live up to that, both in TBank (where you would remove the New from TBank.Create) and outside of TBank (where you would never reference BankCredit again once you passed in to TBank.Create).
An alternate style would be:
(* The TBank.Create constructor's parameter is a pointer which must point to valid memory and must remain valid until after the TBank object is destoryed at which time it is your responsibility to dispose the pointer if required *)
and then live up to that, both in TBank (where the New and Dispose need to be removed) and your main program (which is correct except that the writeln should presumably be writeln(BankCredit^)).
[I've included the full original program below for completeness (sorry to any top-post-haters)]
Note that there is one very important and yet subtle difference between the two strategies. In the first strategy, the pointer must be allocated in a specific way such that TBank can know how to dispose of the pointer. In the second strategy, TBank only needs to get a valid pointer, it does not care how the pointer was allocated (so it could be an @BankCreditVariable, or could be created using a system routine like malloc or Mac's NewPtr or whatever). this is important because you can only Dispose a pointer that is created with New but you can get pointers in other ways (such as from a system library routine) that are still valid pointers and still point to memory, but cannot be disposed using Dispose.
As for how to avoid these sorts of errors, there are a bunch of methods:
Always initialize your variables. Always set pointers to nil once they are disposed. Never keep a pointer recorded in two different locations. And most importantly, heavily use assertions.
I highly recommend reading Writing Solid Code by Steve Maguire.
http://www.peter.com.au/books.html#WritingSolidCode
If you have not read this book or internalized the concept of using assertions and asking for every bug you find, "How could I have avoided introducing that bug?" then you should seriously consider reading or re-reading it.
Enjoy, Peter.
At 23:51 +0000 12/6/04, Neil Santos wrote:
program BankTest; varBankCredit : PCredit; BankSample : PBank;
beginNew(BankCredit); BankCredit^ := 1000;
New(BankSample, BankCredit); BankSample^.Destroy; writeln(BankSample^); Dispose(BankCredit); end.
TBank.Create has this:
constructor TBank.Create(amount : PCredit); beginNew(Self.AccountBalance); Self.AccountBalance := amount; end;
TBank.Destroy has this:
destructor TBank.Destroy; beginDispose(Self.AccountBalance); end;
On 15:35 13/06/04, Peter N Lewis wrote:
constructor TBank.Create(amount : PCredit); begin New(Self.AccountBalance); Self.AccountBalance := amount; end;
This pair of lines shows a deep lack of understanding of what a pointer is and how pointers work.
The first line allocates some memory for you and puts the address of the memory into the pointer variable Self.AccountBalance.
Yes, of course. My sincerest apologies.
-
Adriaan van Os -
Frank Heckenbach -
Neil Santos -
Peter N Lewis