Recent

Author Topic: Sloppy and stupid coding found in LCL.  (Read 7769 times)

nanobit

  • Full Member
  • ***
  • Posts: 160
Re: Sloppy and stupid coding found in LCL.
« Reply #15 on: December 18, 2018, 07:55:00 pm »
Compiling the LCL with parameter -CR should help for code analysis.
- At compile time, -CR prevents wrong typecasts: 
TNonChildClass( baseObj).someMethod
- At runtime, TsomeClass(c) becomes (c as TsomeClass)
with runtime error 219 (Invalid typecast)

That's why I recently submitted:
https://bugs.freepascal.org/view.php?id=34597
https://bugs.freepascal.org/view.php?id=34601

This again led to finding this:
https://bugs.freepascal.org/view.php?id=34605
which should make the LCL usable with -CR.

jamie

  • Hero Member
  • *****
  • Posts: 6090
Re: Sloppy and stupid coding found in LCL.
« Reply #16 on: December 18, 2018, 11:11:12 pm »
I think using the "is" or "as" is a perfect solution to ensure property class identity especially when you are performing such casting
tricks.

 I don't mind casting tricks because I do them a lot to gain access to otherwise hidden members but when you start adding data fields
there should be specific comments to ensure anyone not aware of what is going on to ensure they don't decide to refactor the code for a cleaner
look. Also of course when doing such tricks you must ensure you create the class with that cast so the resources will be allocated.

 I would bet at one time this code did work and somewhere some code refactor took place.

 a compiler warning should be generated at the minimum but not reject the code because I do see merit in the coding method but it has to be
well documented in the source code when used.
 
Thanks guys, you're on the ball! :D
The only true wisdom is knowing you know nothing

BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Sloppy and stupid coding found in LCL.
« Reply #17 on: December 19, 2018, 09:31:43 am »
In attachment a string of patches relative to the lazarus dir that seem to correct the problems. At least for me.

As an aside, some inherited create where dropped because they only add much useless code and fat, specifically such as generating unused Try / except / end in the constructor create procedure.

procedure SetCentered is replaced with property HotSpotPos that can be one off

THotSpots = (DefaultPos,
             LftTop,      MiddleTop,   RightTop,
             LeftMiddle,    Center,    ReightMiddle,
             LeftBottom, MiddleBottom, RightBottom);

Hope patch is complete, only tested on Win 10, use at your own risks.

wp

  • Hero Member
  • *****
  • Posts: 11855
Re: Sloppy and stupid coding found in LCL.
« Reply #18 on: December 19, 2018, 09:58:31 am »
Please post your patch with an appropriate report in the bug tracker so that relationships to other bugs can be established and documented (e.g, https://bugs.freepascal.org/view.php?id=26945).

BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Sloppy and stupid coding found in LCL.
« Reply #19 on: December 19, 2018, 10:34:12 am »
@wp
I have no time  to waste. You, or someone else, test and act accordingly.

wp

  • Hero Member
  • *****
  • Posts: 11855
Re: Sloppy and stupid coding found in LCL.
« Reply #20 on: December 19, 2018, 10:50:31 am »
I have no time  to waste.
You already did. After a few days, when nobody will post to this thread any more, your patch will be buried underneath hundreds of other posts. This is the risk when you bypass the official tools, this works only if the maintainer of the related subject is active in the forum.

And BTW, I have no time to waste either.

AlexTP

  • Hero Member
  • *****
  • Posts: 2386
    • UVviewsoft
Re: Sloppy and stupid coding found in LCL.
« Reply #21 on: December 19, 2018, 11:17:43 am »
TmyCastClass(BM).variable:= -1; //comment out this line for a test;

It is bug in your code, because you cannot cast BM to the class which is descendant of BM type.

Thaddy

  • Hero Member
  • *****
  • Posts: 14201
  • Probably until I exterminate Putin.
Re: Sloppy and stupid coding found in LCL.
« Reply #22 on: December 19, 2018, 11:39:43 am »
Yes you can only down-cast, not up-cast: not enough information. E.g. You can not cast a TObject to TComponent......and expect it to work....
Specialize a type, not a var.

jamie

  • Hero Member
  • *****
  • Posts: 6090
Re: Sloppy and stupid coding found in LCL.
« Reply #23 on: December 20, 2018, 12:16:41 am »
You guys make me laugh... Really

Please scroll back and read the thread from the beginning because you are making assumptions before you know the facts

I presented a test case for public option and I got a lot, however, this test case is an example of what is currently being done in
the LCL code at the present of a section of code. Most likely more cases like this in the LCL. I have a belief, its like mice, where there is
one there are many elsewhere.

 Technically speaking you can cast upward to gain access to hidden members or even add some methods, and the other, well, this is
where the problem is created. When adding field members you now do not have the needed memory allocated for this resource but it
can be done if the class instance was created using this type of casting which allocates the needed extra space and it does work, I've done it
many times using old Delphi and it seems to work with Fpc too.

 The point is, when this kind of coding is performed to take shortcuts there should be tons of comments noting that the object being casted upwards
was created as such with the same casting class.

 So what you have is a class instance that is being used as is but has the additional heap memory. This memory gets cleaned up using the destructor
from any level so it works a treat, but like I said "Comments are needed on the code" because evidently someone must of refactor the code without
knowing. I find it hard to believe it never worked, I find it easier to believe that it worked at one time and good cleaned up without testing it afterwards.



The only true wisdom is knowing you know nothing

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Sloppy and stupid coding found in LCL.
« Reply #24 on: December 20, 2018, 12:52:57 am »
Actually what is missing IMHO:

1) Testcases.

2) asserts or IFDEF conditional checks with "raise" (depending on data origin).
Though not necessarily in the place of the type cast. There it is to late (and can be done by compilerflag)
   TSharedIcon.Add
This should (at least if compiled with asserts/raise) check the added image against the required class.


Also
TSharedIcon(FSharedImage).FImages.Add()
is very wrong.
it is just be "TSharedIcon(FSharedImage).Add();"




BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Sloppy and stupid coding found in LCL.
« Reply #25 on: December 20, 2018, 10:21:40 am »
The bad transtyping was introduced in

Revision: 46685
Author: juha
Date: dimanche, 26 octobre 2014 21:36:37
Message:
LCL: Implement TCursorImage.SetHotSpot. Issue #26945, patch from Denis Kozlov.
----
Modified : /trunk/lcl/graphics.pp
Modified : /trunk/lcl/include/cursorimage.inc

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Sloppy and stupid coding found in LCL.
« Reply #26 on: December 20, 2018, 12:26:49 pm »
In attachment a string of patches relative to the lazarus dir that seem to correct the problems. At least for me.

I am not sure what your patch tries to do?

1) there is a comment about TCursorImageImage
Quote
{ ~bk Useless ! It is never created and wrong transtyping causes IDE to die.
TCursorImageImage is created.

There were however 3 locations were it was not created by mistake. This is fixed in svn revision 59851

The existing typecast in SetHotSpot is correct.
It may stand to argument if it should use "as", "assert" or similar. IMHO that check should be done at a different place.
It may also be an idea to factor out some of the typecasting into a new property getter.

2) Changes to functionality
You introduce "property HotSpotPos". IMHO nice idea.
But as stated already that needs to go through the bugtracker. And as a patch independent to any fixes regarding the sub-class usage.

BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Sloppy and stupid coding found in LCL.
« Reply #27 on: December 21, 2018, 10:37:41 am »
I am not sure what your patch tries to do?
Quote
Quote
{ ~bk Useless ! It is never created and wrong transtyping causes IDE to die.
That was the first problem that jamie had that I could reproduce (even on the first pass with heaptrc) because the bad transtyping did write in the heap memory.
TCursorImageImage is created.
No it is not created as a TCursorImageImage, memory is allocated using the correct instance size, but TCursorImageImage.Create(const AInfo: TIconInfo);, line 2817 of graphics.pp is never called, you can put a break point on it. So the behaviour is pretty indeterminable, it is just an additional class that does not bring any improvement (but adds one more class with the compiler generated code block and nothing really useful).

TCursorImage should be removed and the HotSpot fields should be stored, either in fields to add to TIconImage or, more consistently, since we deal with the hotspot of a cursor,  in TCursorImage.
 
The other changes I made was removing Inherited for some classes that didn't need them. Every time inherited is not uselessly called saves some hundreds of cpu cycles, constructor create is quite fat. (Trying to fight against so said Witrh's law https://en.wikipedia.org/wiki/Wirth%27s_law)

There were however 3 locations were it was not created by mistake. This is fixed in svn revision 59851
That "corrects" the SIGSEG but I don't understand what the class itself is expected to do.

2) Changes to functionality
You introduce "property HotSpotPos". IMHO nice idea.
But as stated already that needs to go through the bugtracker. And as a patch independent to any fixes regarding the sub-class usage.
Just for fun, except for the misspelled LeftTop, it adds an interesting feature and as they say, it is free software. For the time being I have no use for TCursorImage but who knows in the future.


Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 9792
  • Debugger - SynEdit - and more
    • wiki
Re: Sloppy and stupid coding found in LCL.
« Reply #28 on: December 21, 2018, 02:05:15 pm »
TCursorImageImage is created.
No it is not created as a TCursorImageImage, memory is allocated using the correct instance size, but TCursorImageImage.Create(const AInfo: TIconInfo);, line 2817 of graphics.pp is never called, you can put a break point on it. So the behaviour is pretty indeterminable, it is just an additional class that does not bring any improvement (but adds one more class with the compiler generated code block and nothing really useful).
Ah, I missed that info. Though I strongly disagree with the wording.

There is a difference between "a class being created" and "a (specific) contructor being (not) called".
A class can be created, even if it has no constructor of its own (TObject.create is always the fallback). Yet the class is created.

So therefore TCursorImageImage is created.

The base class TIconImage has 3 different constructors to choose from.
1)
2 of them take arguments (TPixelFormat, TRawImage) that have no Hotspot info. So the constructor in TCursorImageImage has nothing to do in that case.

2)
2 of them are not virtual, and do not call the virtual one. That indeed indicates a serious design issue.

Generally the concept of virtual constructors, and offering multiple constructors (that do not call each other), is a problem too. As it means the child class will have to override them all.
There should be one constructor, that is called by all of them. Or alternatively an "Init; virtual" method.

So yes that does need further review.

As for how to fix, I can not tell yet. For that the original intent has to be deducted (svn blame)


Quote
TCursorImage should be removed and the HotSpot fields should be stored, either in fields to add to TIconImage or, more consistently, since we deal with the hotspot of a cursor,  in TCursorImage.
TCursorImage  or TCursorImageImage? I assume the latter?

Can't comment until further research.
But the instance is stored on FImages, suggesting there could be more than one (not sure that can happen for TCursorImage?). If that can happen, then there is more than one Hotspot.
Also need to review towards backward compatibility

 
Quote
The other changes I made was removing Inherited for some classes that didn't need them. Every time inherited is not uselessly called saves some hundreds of cpu cycles, constructor create is quite fat. (Trying to fight against so said Witrh's law https://en.wikipedia.org/wiki/Wirth%27s_law)
Need to check that. Partly its a general design decision.  It all comes down, if there is a chance that ever anything could be added into the now empty constructor.

---------
I am not sure when or if I get to dive into those details (hence the request to put all of it on the bugtracker).

Different people on the team, look after different parts of the code. (And occasionally step in, outside there main area). My main area is SynEdit and the debugger(s).

BrunoK

  • Sr. Member
  • ****
  • Posts: 452
  • Retired programmer
Re: Sloppy and stupid coding found in LCL.
« Reply #29 on: December 21, 2018, 05:04:18 pm »
Different people on the team, look after different parts of the code. (And occasionally step in, outside there main area). My main area is SynEdit and the debugger(s).
First things first. Thanks a lot for the great job your are doing on those. I really enjoy both, it is a pleasure to use Lazarus.
As for how to fix, I can not tell yet. For that the original intent has to be deducted (svn blame)
The current revision 59851 is enough to prevent the SIGSEV so it is acceptable.
TCursorImage  or TCursorImageImage? I assume the latter?
No the prior. In my patch, procedure TCursorImage.HandleNeeded; takes care of setting the correct hotspot when calling FSharedImage.FHandle := WidgetSet.CreateIconIndirect(@lIconInfo);

As to what constructor create does this is what I have found tracing with YOUR debugger and tracing the assembler generated code. I am not a specialist but that is what I have understood :
Code: Pascal  [Select][+][-]
  1.   { Pseudo code as generated by the compiler in unit compiler\psub.pas FPC 3.0.4
  2.     for ANY constructor declared. The fallback being the Parent class Constructor,
  3.     in 90 % of the cases TObject.create
  4.  
  5.     COMPILER searches and finds the correct constructor (correct matching parameter)
  6.     tracking constructors from TSomeClass up to TClass.
  7.  
  8.     procedure tcgprocinfo.maybe_add_constructor_wrapper(var tocode: tnode; withexceptblock: boolean);
  9.     parameters are  :
  10.       aObject : TObject if aNew=0 or TClass if aNew=1  (In %eax in win 32)
  11.       aNew    : create instance if = 1 do NewInstance stuf (In %edx in win 32)
  12.                                 if = use aObject instance
  13.  
  14.     result in %eax, a pointer to the block of allocated memory for the
  15.       TSomeClassInstance.create in main program (or nil if failure)
  16.  
  17.     Recusively called for parent class when inherited is called until TObject
  18.     of another parent class returns }
  19.   function GenericCreate(aObject: TObject, aNew : integer, aParams : things on stack):TSomeClassInstance;
  20.   var
  21.     lSelf : TObject;
  22.   begin
  23.     lSelf := aObject;
  24.     if aNew=1 then                // if 1 : lSelf is aObject.ClassRef
  25.       lSelf := lSelf.NewInstance;  // Virtual NewInstance take TSomeClass.InstanceSize
  26.                                   // to allocate memory from getmem. In 99% of
  27.                                   // class it is TObject.NewInstance.
  28.     if lSelf = nil then           // Error, return nil instance
  29.       Exit(nil);
  30.     aNew = 0;                     // lSelf is now an instance
  31.     { This try / except / end is quite fat and always generated even if there is
  32.       nothing to do. }
  33.     try
  34.       { Code for body of TSomeClass.Create inserted here by compiler
  35.         in maybe_add_constructor_wrapper,
  36.         That's YOUR CODE IF ANY, may be empty
  37.         if inherited is called as in
  38.           inherited create; -> the code generated is :
  39. >>      GenericCreate(Self, aNew : integer=0 at this stage <<< }
  40.       { After construction code }
  41.       lSelf.AfterConstruction; // There are a very limited number of classes
  42.                               // overriding TObject.AfterConstruction in FPC and
  43.                               // Lazarus.
  44.                               // Here is the beauty of VIRTUAL methods of
  45.                               // delphi/free pascal. It calls whatever is
  46.                               // in the TSomeClass or one of its ParentClass and generally the fallback is
  47.                               // TObject.AfterConstruction
  48.     except
  49.       if Assigned(lSelf) then
  50.         lSelf.Destroy;
  51.       lSelf:=nil;
  52.     end;
  53.     Result := lSelf;
  54.   end;
  55.  

 

TinyPortal © 2005-2018