Re: bitpos expansion patches summary

[Jan Kratochvil <jan.kratochvil@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4072 bytes --]

On Fri, 07 Sep 2012 12:51:58 +0200, Siddhesh Poyarekar wrote:
> On Sun, 2 Sep 2012 20:15:15 +0200, Jan wrote:
> > I was thinking about some updating of file:lineno records in
> > '.locdiff.report' file to their new location and then diffing the
> > old+reviewed '.locdiff.report' against new file.  But I have not
> > tried it yet in practice.
> 
> This will be heuristic to some extent, which is why I avoided it. 

The current locdiff output is already heuristic.  With the attached source:

perl -pe 's/\Q{+\E(.*)?\Q+}\E/$1/g;s/\Q[-\E(.*)?\Q-]\E//g' <5.s >5p.c;perl -pe 's/\Q{+\E(.*)?\Q+}\E//g;s/\Q[-\E(.*)?\Q-]\E/$1/g' <5.s >5m.c;diff -u 5m.c 5p.c;for i in 5m 5p;do gcc -o $i $i.c -Wall -g;splint -showcolumn $i.c|sed "s/$i[.]c/5.c/"|perl -lpe 's/^(\s*)(\S*?\.[ch]:\d+):/$1LOC $2\n/'|tee $i.splint|perl -lpe 's{^(\s*)LOC (.*)$}{$1LOC}' >$i.splintloc;done;diff -U-1 5{m,p}.splintloc;perl -le 'print "-"x80';/tmp/splint-bitpos/splint-locdiff 5{m,p}.splint

as explanation of the script above the source line
  [-int-]{+long+} len;
gets translated in the two sources accordingly:
  int len;
->
  long len;

--- 5m.splintloc	2012-09-11 20:46:27.727578194 +0200
+++ 5p.splintloc	2012-09-11 20:46:27.785578114 +0200
@@ -1,11 +1,11 @@
 5.c: (in function stub1)
 LOC
  Variable stubj initialized to type long int, expects int: stubi
   To ignore type qualifiers in type comparisons use +ignorequals.
-5.c: (in function f)
+5.c: (in function g)
 LOC
  Assignment of long int to int: b.len = a.len
 5.c: (in function stub2)
 LOC
  Variable stubj initialized to type long int, expects int: stubi
 
--------------------------------------------------------------------------------
LOC 5.c:6
 Variable stubj initialized to type long int, expects int: stubi
  To ignore type qualifiers in type comparisons use +ignorequals.
LOC 5.c:49
 Variable stubj initialized to type long int, expects int: stubi


The first part shows there is only very minor difference of the splint output
while the splint-locdiff already contains no traces of it.

And still the change caused a type safety regression in function g.

I hope such cases do not happen in real but it is still a heuristic hope only.


How do you want to check it in without doing several rebases?

Also I have found several missed expansions only by hand, one needs to do full
re-run of splint on the patched sources.  As the patched sources change line
numbers a bit it already means some sort of rebase.

Also we have now about a month old analysis, all the code checked in the last
month may be broken.

Additionally I am pretty sure the codebase will get broken soon again as it is
common GDB practice to use 'int' for every length and I do not review very
every check-in.  So it would be nice to possibly be able to do such
incremental re-checks in the future; although I am not sure it will be done.


Also IMO (any feedback from other maintainers?) we need full annotation of the
patch file as with such large number of change there is not clear which
changes are justified and whether there are no excessive changes.
	http://people.redhat.com/jkratoch/bitpos3.patch
	(lines starting with 'x')


> Probably a case of different line numbers due to using a different
> updated version? I was rebasing every now and then, which gave me
> different line numbers for some warnings. I see this on line 4695 now.

I stay with 2636a39d8bf9b24dce328e4f906e8710b52d2105 which is a commit very
close to the time you generated the output.

We should always stick to a single commit and document which commit, otherwise
it is difficult to interchange the data.


I will reply to the specific items in next mail.  Then we need some new full
run of splint on patched sources and reuse the existing reviews in some
hauristic way.

And to complete the patch annotation.  I wrote it in a way to make it
automatically cross-checkable.  That each 'x' line in the patch has
corresponding 'report' line and that each FIXED 'report' line has at least one
'x' line in the patch.  I still have no script for checking the 'x' lines.


Thanks,
Jan

[-- Attachment #2: 5.s --]
[-- Type: text/plain, Size: 531 bytes --]

static long used;

static void
stub1 (long stubi)
{
  int stubj = stubi;

  used = stubj;
}

struct s1
{
  long len;
};

struct s2
{
  [-int-]{+long+} len;
};

struct s3
{
  int len;
};

static void
f (void)
{
  struct s1 a = { 0 };
  struct s2 b;

  b.len = a.len;
  used = b.len;
}

static void
g (void)
{
  struct s2 a = { 0 };
  struct s3 b;

  b.len = a.len;
  used = b.len;
}

static void
stub2 (long stubi)
{
  int stubj = stubi;

  used = stubj;
}

int main (void)
{
  stub1 (0);
  stub2 (0);
  f ();
  g ();
  return 0;
}