View Issue Details

IDProjectCategoryView StatusLast Update
0038680FPCCompilerpublic2021-04-20 16:46
ReporterErich Eckner Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status newResolutionopen 
PlatformLinuxOSArch Linux 
Product Version3.3.1 
Summary0038680: wrong overloaded method is chosen, when they differ only in pointer to specialized class
DescriptionI have two specialized classes and pointer types to them. These classes contain overloaded functions, which take a variable of either pointer type as argument. However, it is always the first method, that is being called. It does not help to explicitly cast the pointer on the call, either.

This leads to trouble, because the de-referenced pointers behave differently (e.g. differing sizeof()).
Steps To Reproducetake the attached program and do

> fpc -MObjFPC mwe.pas
> ./mwe
Additional InformationThis definitely worked before (maybe not exactly my minimal working example, but the actual program which uses similar structures), but I am unable to find, when exactly it broke - apparently, it was broken in 3.2.0 already ...
Tagsgenerics
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Relationships

has duplicate 0038679 resolvedSven Barth calls wrong overloaded function if difference is only in type of pointer to specialized class 

Activities

Erich Eckner

2021-03-29 10:03

reporter  

mwe.pas (1,102 bytes)   
program mwe;

uses sysutils;

type
  pArraySingle = ^tArraySingle;
  pArrayDouble = ^tArrayDouble;
  generic tArray<prec> = class(tObject)
    vals: array of prec;
    constructor create;
    procedure dumpInfo(prefix: string = '');
    procedure copy_from(a: pArraySingle); overload;
    procedure copy_from(a: pArrayDouble); overload;
  end;
  tArraySingle = specialize tArray<single>;
  tArrayDouble = specialize tArray<double>;

constructor tArray.create;
begin
  inherited create;
  setlength(vals,1);
  vals[0]:=42;
end;

procedure tArray.dumpInfo(prefix: string);
begin
  writeln(prefix+ClassName);
  writeln(prefix+inttostr(sizeof(vals[0])));
end;

procedure tArray.copy_from(a: pArraySingle);
begin
  tArraySingle(a^).dumpInfo('Single Original: ');
end;

procedure tArray.copy_from(a: pArrayDouble);
begin
  tArrayDouble(a^).dumpInfo('Double Original: ');
end;

var
  ar1,ar2: tArrayDouble;

begin
  ar1:=tArrayDouble.create;
  ar2:=tArrayDouble.create;
  ar1.dumpInfo('ar1: ');
  ar2.dumpInfo('ar2: ');
  ar2.copy_from(@ar1);
  ar2.copy_from(pArrayDouble(@ar1));
  ar1.free;
  ar2.free;
end.
mwe.pas (1,102 bytes)   

Bart Broersma

2021-03-29 11:20

reporter   ~0129972

Duplicate of 0038679

Do-wan Kim

2021-04-02 08:30

reporter   ~0130035

This patch works with i386-win32 and win64.
checking exact count first.
38680_exact_first_htypechk.pas.patch (8,094 bytes)   
Index: compiler/htypechk.pas
===================================================================
--- compiler/htypechk.pas	(revision 49100)
+++ compiler/htypechk.pas	(working copy)
@@ -2892,6 +2892,7 @@
         obj_to   : tobjectdef;
         def_from,
         def_to   : tdef;
+        tempn,
         currpt,
         pt       : tcallparanode;
         eq,
@@ -3001,7 +3002,10 @@
                   is_integer(def_to) and
                   is_in_limit(def_from,def_to) then
                  begin
-                   eq:=te_equal;
+                   if torddef(def_from).ordtype=torddef(def_to).ordtype then
+                     eq:=te_exact
+                     else
+                       eq:=te_equal;
                    hp^.ordinal_distance:=hp^.ordinal_distance+
                      abs(bestreal(torddef(def_from).low)-bestreal(torddef(def_to).low));
                    rth:=bestreal(torddef(def_to).high);
@@ -3023,7 +3027,10 @@
                   is_real_or_cextended(def_from) and
                   is_real_or_cextended(def_to) then
                  begin
-                   eq:=te_equal;
+                   if tfloatdef(def_from).floattype=tfloatdef(def_to).floattype then
+                     eq:=te_exact
+                     else
+                       eq:=te_equal;
                    if is_extended(def_to) then
                      rth:=4
                    else
@@ -3130,6 +3137,30 @@
                       para_allowed(eq,currpt,def_to);
                   end;
                end;
+              { to specialize pointer }
+              if is_pointer(def_from) and is_pointer(def_to) and
+                 (tstoreddef(tpointerdef(def_to).pointeddef).is_specialization) then
+                 begin
+                   if equal_defs(tpointerdef(def_from).pointeddef,tpointerdef(def_to).pointeddef)
+                   then
+                     eq:=te_exact
+                     else
+                       { void pointer }
+                       if currpt.left.nodetype=addrn then
+                       begin
+                         tempn:=tcallparanode(currpt.left);
+                         while assigned(tempn) do
+                         begin
+                           if tempn.nodetype in [loadn,derefn] then
+                           begin
+                             if equal_defs(tempn.resultdef,tpointerdef(def_to).pointeddef) then
+                               eq:=te_exact;
+                             break;
+                           end;
+                           tempn:=tcallparanode(tempn.left);
+                         end;
+                       end;
+                 end;
 
               { univ parameters match if the size matches (don't override the
                 comparison result if it was ok, since a match based on the
@@ -3291,53 +3322,53 @@
            res:=(bestpd^.coper_count-currpd^.coper_count);
            if (res=0) then
             begin
-             { less cl6 parameters? }
-             res:=(bestpd^.cl6_count-currpd^.cl6_count);
+             { more exact parameters? }
+             res:=(currpd^.exact_count-bestpd^.exact_count);
              if (res=0) then
               begin
-                { less cl5 parameters? }
-                res:=(bestpd^.cl5_count-currpd^.cl5_count);
-                if (res=0) then
-                 begin
-                  { less cl4 parameters? }
-                  res:=(bestpd^.cl4_count-currpd^.cl4_count);
+               { less cl6 parameters? }
+               res:=(bestpd^.cl6_count-currpd^.cl6_count);
+               if (res=0) then
+                begin
+                  { less cl5 parameters? }
+                  res:=(bestpd^.cl5_count-currpd^.cl5_count);
                   if (res=0) then
                    begin
-                    { less cl3 parameters? }
-                    res:=(bestpd^.cl3_count-currpd^.cl3_count);
+                    { less cl4 parameters? }
+                    res:=(bestpd^.cl4_count-currpd^.cl4_count);
                     if (res=0) then
                      begin
-                       { less cl2 parameters? }
-                       res:=(bestpd^.cl2_count-currpd^.cl2_count);
-                       if (res=0) then
-                        begin
-                          { less cl1 parameters? }
-                          res:=(bestpd^.cl1_count-currpd^.cl1_count);
-                          if (res=0) then
-                           begin
-                             { more exact parameters? }
-                             res:=(currpd^.exact_count-bestpd^.exact_count);
-                             if (res=0) then
-                              begin
-                                { less equal parameters? }
-                                res:=(bestpd^.equal_count-currpd^.equal_count);
-                                if (res=0) then
-                                 begin
-                                   { smaller ordinal distance? }
-                                   if (currpd^.ordinal_distance<bestpd^.ordinal_distance) then
-                                    res:=1
-                                   else
-                                    if (currpd^.ordinal_distance>bestpd^.ordinal_distance) then
-                                     res:=-1
-                                   else
-                                    res:=0;
-                                 end;
-                              end;
-                           end;
-                        end;
+                      { less cl3 parameters? }
+                      res:=(bestpd^.cl3_count-currpd^.cl3_count);
+                      if (res=0) then
+                       begin
+                         { less cl2 parameters? }
+                         res:=(bestpd^.cl2_count-currpd^.cl2_count);
+                         if (res=0) then
+                          begin
+                            { less cl1 parameters? }
+                            res:=(bestpd^.cl1_count-currpd^.cl1_count);
+                            if (res=0) then
+                             begin
+                              { less equal parameters? }
+                              res:=(bestpd^.equal_count-currpd^.equal_count);
+                              if (res=0) then
+                               begin
+                                 { smaller ordinal distance? }
+                                 if (currpd^.ordinal_distance<bestpd^.ordinal_distance) then
+                                  res:=1
+                                 else
+                                  if (currpd^.ordinal_distance>bestpd^.ordinal_distance) then
+                                   res:=-1
+                                 else
+                                  res:=0;
+                               end;
+                             end;
+                          end;
+                       end;
                      end;
                    end;
-                 end;
+                end;
               end;
             end;
          end;
@@ -3608,7 +3639,7 @@
           cpoptions:=cpoptions+[cpo_rtlproc];
 
         compare_by_old_sortout_check := 0; // can't decide, bestpd probably wasn't sorted out in unpatched
-        if (compare_paras(pd^.data.paras,bestpd^.data.paras,cp_value_equal_const,cpoptions)>=te_equal) and
+        if (compare_paras(pd^.data.paras,bestpd^.data.paras,cp_value_equal_const,cpoptions)>te_equal) and
           (not(po_objc in bestpd^.data.procoptions) or (bestpd^.data.messageinf.str^=pd^.data.messageinf.str^)) then
           compare_by_old_sortout_check := 1; // bestpd was sorted out before patch
      end;
@@ -3701,6 +3732,9 @@
                      res:=is_better_candidate(hp,besthpstart)
                    else
                      res:=is_better_candidate_single_variant(hp,besthpstart);
+                   { avoid valid both }
+                   if (res=0) and (cntpd>0) then
+                     res:=1;
                  end;
                  if restart then
                    begin

Jan Bruns

2021-04-03 17:05

reporter   ~0130063

Without having focussed on that issue: Could you decribe your findings a bit more?

Was one of the example functions really sorted out in compare_by_old_sortout_check()?

Your patch also adds the the code

if (res=0) and (cntpd>0) then res:=1;

which doesn't look like a real/acceptable solution to me. In clear words, it reads: "if there are multiple valid candidates, and both are equally matching, choose at random" instead of telling the calling code about that situation (to make it display an error).

Do-wan Kim

2021-04-03 17:32

reporter   ~0130065

Last edited: 2021-04-03 17:34

View 2 revisions

compare_by_old_sortout_check() function may not work properly above example.
I got both overload function with patch and compare parameters properly.

"if (res=0) and (cntpd>0) then res:=1"
besthpstart point walks one by one in candidate list. It makes step bottom in list, not random.
I guess it compatible with previous candidate overload selection.

My patch is not perfect solution.
But it compiles no error with trunk FPC and trunk lazarus on i386-win32 and x86-64-win64.

Jan Bruns

2021-04-03 18:22

reporter   ~0130066

That's interesting (but sounds like a florian-kind of prob).

Well, not random, the order of candidates reflects rough concerns about identifier visibility, but otherwise might for example depend on the order the procedures have been declared in source code. Don't expect this list to already be sorted by degree of match with the call node.

Do-wan Kim

2021-04-04 01:17

reporter   ~0130070

Last edited: 2021-04-04 02:04

View 2 revisions

Main problem is in 'is_better_candidate' function.
After patch, it pick up every good exact match candidate first.
I changed overload definition order in other test example, it always choose good candidate.

Bad scenario is same match parameter comparison levels with candidate functions but it is rare to occur.

If you interesting to test, precompiled fpc i386-win32 trunk following link (about 90MiB).
https://mega.nz/file/ZdAzTQia#U1rPIHhzcWM1RSsLpfIP7jEaSsR5Nit4dQBKHf7EYSs

Jan Bruns

2021-04-06 03:23

reporter   ~0130127

Does your patch probably already work without modding the sortout?

Let me tell once again: Up to relatively recent changes (bug 0036666), this hopefully very same sortout happend in "create_candidate_list".

It doesn't really have to do with choosing a candidate. It's about visibility blending, which, well, true, in turn works by comparing candidates, but only for the purpose of finding out wether a candidate is visible at all.

Visibility shouldn't be a problem here, because if so, plus knowledge that the 2 overloads should share the same visibility, the class declaration check would/should also through an error.

Do-wan Kim

2021-04-06 06:26

reporter   ~0130132

When disabled sortout modifications, my patch generate error on Gerenics RTL compiling.
There is missing point I don't notice yet.

Jan Bruns

2021-04-06 14:52

reporter   ~0130136

Given fpc3.2 compiles the example without error indication, it seems the same-visibility overloads were different enough to not generate an "impossible overload" error, and also no "can't decide call target"-error.

So my guess is the overloads didn't sort-out against each other, and had a clear but buggy match relation w.r.t. to the call node. Maybe the real problem is a shortcoming related to recursive ptr-type-def comparison (shouldn't the lesser preferable overload be marked incompatible?).

Whatever it is, it seems I cannot help!

Do-wan Kim

2021-04-07 07:33

reporter   ~0130149

Refined patch. More clear on compatiblity checking.
In patch , code changes to
' if (compare_paras(pd^.data.paras,bestpd^.data.paras,cp_value_equal_const,cpoptions)>te_equal) and'
for preventing maked invalid first overload function.
38680_refined_htypechk.pas-2.patch (8,526 bytes)   
Index: compiler/htypechk.pas
===================================================================
--- compiler/htypechk.pas	(리비전 49131)
+++ compiler/htypechk.pas	(작업 사본)
@@ -2892,11 +2892,15 @@
         obj_to   : tobjectdef;
         def_from,
         def_to   : tdef;
+        tempn,
         currpt,
         pt       : tcallparanode;
+        eqtemp,
         eq,
         mineq    : tequaltype;
+        nconvtyp,
         convtype : tconverttype;
+        npd,
         pdtemp,
         pdoper   : tprocdef;
         releasecurrpt : boolean;
@@ -3001,7 +3005,10 @@
                   is_integer(def_to) and
                   is_in_limit(def_from,def_to) then
                  begin
-                   eq:=te_equal;
+                   if torddef(def_from).ordtype=torddef(def_to).ordtype then
+                     eq:=te_exact
+                     else
+                       eq:=te_equal;
                    hp^.ordinal_distance:=hp^.ordinal_distance+
                      abs(bestreal(torddef(def_from).low)-bestreal(torddef(def_to).low));
                    rth:=bestreal(torddef(def_to).high);
@@ -3023,7 +3030,10 @@
                   is_real_or_cextended(def_from) and
                   is_real_or_cextended(def_to) then
                  begin
-                   eq:=te_equal;
+                   if tfloatdef(def_from).floattype=tfloatdef(def_to).floattype then
+                     eq:=te_exact
+                     else
+                       eq:=te_equal;
                    if is_extended(def_to) then
                      rth:=4
                    else
@@ -3130,6 +3140,30 @@
                       para_allowed(eq,currpt,def_to);
                   end;
                end;
+              { to specialize pointer }
+              if (eq<te_exact) and
+                 is_pointer(def_from) and is_pointer(def_to) and
+                 (tstoreddef(tpointerdef(def_to).pointeddef).is_specialization) then
+                 begin
+                   eqtemp:=compare_defs_ext(tpointerdef(def_from).pointeddef,tpointerdef(def_to).pointeddef,nothingn,nconvtyp,npd,[cdo_equal_check]);
+                   if eqtemp<>te_exact then
+                       { void pointer }
+                       if currpt.left.nodetype=addrn then
+                       begin
+                         tempn:=tcallparanode(currpt.left);
+                         while assigned(tempn) do
+                         begin
+                           if tempn.nodetype in [loadn,derefn] then
+                           begin
+                             eqtemp:=compare_defs_ext(tempn.resultdef,tpointerdef(def_to).pointeddef,nothingn,nconvtyp,npd,[cdo_equal_check]);
+                             break;
+                           end;
+                           tempn:=tcallparanode(tempn.left);
+                         end;
+                       end;
+                   if eqtemp=te_exact then
+                     eq:=te_exact;
+                 end;
 
               { univ parameters match if the size matches (don't override the
                 comparison result if it was ok, since a match based on the
@@ -3145,7 +3179,6 @@
                 procvar is choosen. See tb0471 (PFV) }
               if (pt<>currpt) and (eq=te_exact) then
                 eq:=te_equal;
-
               { increase correct counter }
               case eq of
                 te_exact :
@@ -3291,53 +3324,53 @@
            res:=(bestpd^.coper_count-currpd^.coper_count);
            if (res=0) then
             begin
-             { less cl6 parameters? }
-             res:=(bestpd^.cl6_count-currpd^.cl6_count);
-             if (res=0) then
-              begin
-                { less cl5 parameters? }
-                res:=(bestpd^.cl5_count-currpd^.cl5_count);
-                if (res=0) then
-                 begin
-                  { less cl4 parameters? }
-                  res:=(bestpd^.cl4_count-currpd^.cl4_count);
+              { more exact parameters? }
+              res:=(currpd^.exact_count-bestpd^.exact_count);
+              if (res=0) then
+               begin
+               { less cl6 parameters? }
+               res:=(bestpd^.cl6_count-currpd^.cl6_count);
+               if (res=0) then
+                begin
+                  { less cl5 parameters? }
+                  res:=(bestpd^.cl5_count-currpd^.cl5_count);
                   if (res=0) then
                    begin
-                    { less cl3 parameters? }
-                    res:=(bestpd^.cl3_count-currpd^.cl3_count);
+                    { less cl4 parameters? }
+                    res:=(bestpd^.cl4_count-currpd^.cl4_count);
                     if (res=0) then
                      begin
-                       { less cl2 parameters? }
-                       res:=(bestpd^.cl2_count-currpd^.cl2_count);
-                       if (res=0) then
-                        begin
-                          { less cl1 parameters? }
-                          res:=(bestpd^.cl1_count-currpd^.cl1_count);
-                          if (res=0) then
-                           begin
-                             { more exact parameters? }
-                             res:=(currpd^.exact_count-bestpd^.exact_count);
-                             if (res=0) then
-                              begin
-                                { less equal parameters? }
-                                res:=(bestpd^.equal_count-currpd^.equal_count);
-                                if (res=0) then
-                                 begin
-                                   { smaller ordinal distance? }
-                                   if (currpd^.ordinal_distance<bestpd^.ordinal_distance) then
-                                    res:=1
-                                   else
-                                    if (currpd^.ordinal_distance>bestpd^.ordinal_distance) then
-                                     res:=-1
-                                   else
-                                    res:=0;
-                                 end;
-                              end;
-                           end;
-                        end;
+                      { less cl3 parameters? }
+                      res:=(bestpd^.cl3_count-currpd^.cl3_count);
+                      if (res=0) then
+                       begin
+                         { less cl2 parameters? }
+                         res:=(bestpd^.cl2_count-currpd^.cl2_count);
+                         if (res=0) then
+                          begin
+                            { less cl1 parameters? }
+                            res:=(bestpd^.cl1_count-currpd^.cl1_count);
+                            if (res=0) then
+                             begin
+                              { less equal parameters? }
+                              res:=(bestpd^.equal_count-currpd^.equal_count);
+                              if (res=0) then
+                               begin
+                                 { smaller ordinal distance? }
+                                 if (currpd^.ordinal_distance<bestpd^.ordinal_distance) then
+                                  res:=1
+                                 else
+                                  if (currpd^.ordinal_distance>bestpd^.ordinal_distance) then
+                                   res:=-1
+                                 else
+                                  res:=0;
+                               end;
+                             end;
+                          end;
+                       end;
                      end;
                    end;
-                 end;
+                end;
               end;
             end;
          end;
@@ -3608,7 +3641,7 @@
           cpoptions:=cpoptions+[cpo_rtlproc];
 
         compare_by_old_sortout_check := 0; // can't decide, bestpd probably wasn't sorted out in unpatched
-        if (compare_paras(pd^.data.paras,bestpd^.data.paras,cp_value_equal_const,cpoptions)>=te_equal) and
+        if (compare_paras(pd^.data.paras,bestpd^.data.paras,cp_value_equal_const,cpoptions)>te_equal) and
           (not(po_objc in bestpd^.data.procoptions) or (bestpd^.data.messageinf.str^=pd^.data.messageinf.str^)) then
           compare_by_old_sortout_check := 1; // bestpd was sorted out before patch
      end;
@@ -3643,7 +3676,6 @@
         end;
       end;
 
-
     function tcallcandidates.choose_best(var bestpd:tabstractprocdef; singlevariant: boolean):integer;
       var
         pd: tprocdef;

Erich Eckner

2021-04-20 10:53

reporter   ~0130473

(sry for the delay, I activated notifications for this bug thread just now)
@Do-wan Kim: Your patch works for me - also in the bigger project, from which this minimal example was derived.

Issue History

Date Modified Username Field Change
2021-03-29 10:03 Erich Eckner New Issue
2021-03-29 10:03 Erich Eckner File Added: mwe.pas
2021-03-29 11:20 Bart Broersma Note Added: 0129972
2021-03-29 13:03 Sven Barth Tag Attached: generics
2021-04-02 08:30 Do-wan Kim Note Added: 0130035
2021-04-02 08:30 Do-wan Kim File Added: 38680_exact_first_htypechk.pas.patch
2021-04-03 17:05 Jan Bruns Note Added: 0130063
2021-04-03 17:32 Do-wan Kim Note Added: 0130065
2021-04-03 17:34 Do-wan Kim Note Edited: 0130065 View Revisions
2021-04-03 18:22 Jan Bruns Note Added: 0130066
2021-04-04 01:17 Do-wan Kim Note Added: 0130070
2021-04-04 02:04 Do-wan Kim Note Edited: 0130070 View Revisions
2021-04-06 03:23 Jan Bruns Note Added: 0130127
2021-04-06 06:26 Do-wan Kim Note Added: 0130132
2021-04-06 14:52 Jan Bruns Note Added: 0130136
2021-04-07 07:33 Do-wan Kim Note Added: 0130149
2021-04-07 07:33 Do-wan Kim File Added: 38680_refined_htypechk.pas-2.patch
2021-04-20 10:53 Erich Eckner Note Added: 0130473
2021-04-20 16:46 Sven Barth Relationship added has duplicate 0038679