pgindent run coming

Bruce Momjian wrote:

It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
running it at zero-hour GMT tomorrow, meaning five hours from now.
Any objections?

I ran pgindent and was concerned enough about the results so I am
posting here rather than applying any changes. I used the old way of
generating typedefs and the new buildfarm method output from diffs from
http://www.pgbuildfarm.org/cgi-bin/typedefs.pl. The typedef lists and
diff are here:

http://momjian.us/expire/pgindent/

You can see the typedef lists are of similar size:

2775 typedefs.old
2123 typedefs.new

and the diffs generates are a similar number of lines:

133657 diff.old_typedefs
135042 diff.new_typedefs

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

-   ret = stat(indir, &st);
+   ret = stat  (indir, &st);

and even odder:

-                       stat->weight |= 1 << 3;
+                       stat      ->weight |= 1 << 3;

stat was not a typedef in 8.3 or pgindent would have done this for 8.3,
but I can't figure out what has changed to make it appear for 8.4. I
see this in the objdump output (my OS has not changed from 8.3):

31357 EXCL 0 0 00003e64 97648 /usr/include/time.h
31358 EXCL 0 0 00007638 97624 /usr/include/sys/time.h
31359 EXCL 0 0 00000000 97648 /usr/include/time.h
31360 EXCL 0 0 00001dff 25540 /usr/include/fcntl.h
31361 BINCL 0 0 000144be 449667 /usr/include/sys/stat.h

31362 LSYM 0 0 00000000 449691 ostat:T(51,1)=s64st_dev:(0,9),0,16;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st_u
id:(0,9),96,16;st_gid:(0,9),112,16;st_rdev:(0,9),128,16;st_size:(0,3),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(4
8,2),320,64;st_blksize:(0,3),384,32;st_blocks:(0,3),416,32;st_flags:(0,5),448,32;st_gen:(0,5),480,32;;

31363 LSYM 0 0 00000000 450042 stat32:T(51,2)=s96st_dev:(9,17),0,32;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st
_uid:(9,28),96,32;st_gid:(9,19),128,32;st_rdev:(9,17),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(48,2),320,64;st_s
ize:(0,3),384,32;st_size1:(0,3),416,32;st_blocks:(0,3),448,32;st_blocks1:(0,3),480,32;st_blksize:(0,5),512,32;st_flags:(0,5),544,32;st_gen:(0,5),5
76,32;st_lspare:(0,3),608,32;st_qspare:(51,3)=ar(0,1);0;3;(0,3),640,128;;

--> 31364 LSYM 0 0 00000000 450510 stat:T(51,4)=s96st_dev:(9,17),0,32;st_ino:(9,20),32,32;st_mode:(9,22),64,16;st_nlink:(9,23),80,16;st_u
id:(9,28),96,32;st_gid:(9,19),128,32;st_rdev:(9,17),160,32;st_atimespec:(48,2),192,64;st_mtimespec:(48,2),256,64;st_ctimespec:(48,2),320,64;st_siz
e:(9,24),384,64;st_blocks:(9,8),448,64;st_blksize:(0,5),512,32;st_flags:(0,5),544,32;st_gen:(0,5),576,32;st_lspare:(0,3),608,32;st_qspare:(51,5)=a
r(0,1);0;1;(9,8),640,128;;

It is coming from the postgres binary.

The typedef is coming from the indicated line, and from
/usr/include/sys/stat.h, where there is no typedef for stat. Obviously
Linux or the buildfarm is finding the same issue, but I have no idea
why.

My only guess right now is that we are linking postgres differently than
we did for 8.3 and that is bringing in new wrong typedef symbols.

I will have to research this further tomorrow.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

The typedef is coming from the indicated line, and from
/usr/include/sys/stat.h, where there is no typedef for stat. Obviously
Linux or the buildfarm is finding the same issue, but I have no idea
why.

My only guess right now is that we are linking postgres differently than
we did for 8.3 and that is bringing in new wrong typedef symbols.

I will have to research this further tomorrow.

I was able to reproduce the incorrect stat typedef here in a small test
program by just including "sys/stat.h" so I will research tomorrow how
to fix this.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

The problem is that there are other symbols I don't know about and the
diff is very large. I have found that the problem was caused when we
added Linux support to find_typedef and I have a way to get an accurate
list on my machine.

I will generate a proper list on my machine tomorrow then test Linux
here to see if I can get it to generate the right list too. But odds
are we are not going to have time to re-run the list on the build farm
even if I can get Linux working here.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

Do you want me to just run with my old typedef list now and apply it?
We an always rerun tomorrow if we get a better typedef list.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

Do you want me to just run with my old typedef list now and apply it?
We an always rerun tomorrow if we get a better typedef list.

I'd rather have *one* run with the final typedef list. If you don't
have that list yet, wait till you do.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

Do you want me to just run with my old typedef list now and apply it?
We an always rerun tomorrow if we get a better typedef list.

I'd rather have *one* run with the final typedef list. If you don't
have that list yet, wait till you do.

OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
run that. I think that will fix our problem and then I can use the
buildfarm version. How often does that run and does it pull the script
from CVS HEAD?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
run that. I think that will fix our problem and then I can use the
buildfarm version. How often does that run and does it pull the script
from CVS HEAD?

The buildfarm does not run the find-typedefs script. Its code for this
is below. My Unix machine runs this once a day. I can do runs on Windows
and Cygwin manually.

If there is changed logic tell me what it is and I'll try to get it done.

cheers

andrew

-----------------
sub find_typedefs
{
my @err = `objdump -W 2>&1`;
@err = () if `uname -s 2>&1` =~ /CYGWIN/i;
my %syms;
my @dumpout;
my @flds;
foreach my $bin (glob("$installdir/bin/*"),
glob("$installdir/lib/*"),
glob("$installdir/lib/postgresql/*"))
{
next if $bin =~ m!bin/(ipcclean|pltcl_)!;
next unless -f $bin;
if (@err == 1) # Linux
{
@dumpout = `objdump -W $bin 2>/dev/null | egrep -A3
'(DW_TAG_typedef|DW_TAG_structure_type|DW_TAG_union_type)' 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if (($flds[0] ne 'DW_AT_name' && $flds[1] ne 'DW_AT_name' ) ||
$flds[-1] =~ /^DW_FORM_str/);
$syms{$flds[-1]} =1;
}
}
else
{
@dumpout = `objdump --stabs $bin 2>/dev/null`;
foreach (@dumpout)
{
@flds = split;
next if (@flds < 7);
next if ($flds[1] ne 'LSYM' || $flds[6] !~ /([^:]+):[tT]/);
$syms{$1} =1;
}
}
}
my @badsyms = grep { /\s/ } keys %syms;
push(@badsyms,'date','interval','timestamp','ANY');
delete @syms{@badsyms};

my @goodsyms = sort keys %syms;
my @foundsyms;

my %foundwords;

my $setfound = sub
{
return unless (-f $_ && /^.*\.[chly]\z/);
my @lines;
my $handle;
open ($handle,$_);
while (my $line=<$handle>)
{
foreach my $word (split(/\W+/,$line))
{
$foundwords{$word} = 1;
}
}
close($handle);
};

File::Find::find($setfound,"$branch_root/pgsql");

foreach my $sym (@goodsyms)
{
push(@foundsyms,"$sym\n") if exists $foundwords{$sym};
}

writelog('typedefs',\@foundsyms);
$steps_completed .= " find-typedefs";
}

On Tue, 2009-06-09 at 13:21 -0400, Bruce Momjian wrote:

It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
running it at zero-hour GMT tomorrow, meaning five hours from now.
Any objections?

Why don't we do this automatically after each individual commit? That
way each commit would be associated directly with any required tidy-up
that must occur because of it.

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support

Andrew Dunstan wrote:

Bruce Momjian wrote:

OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
run that. I think that will fix our problem and then I can use the
buildfarm version. How often does that run and does it pull the script
from CVS HEAD?

The buildfarm does not run the find-typedefs script. Its code for this
is below. My Unix machine runs this once a day. I can do runs on Windows
and Cygwin manually.

If there is changed logic tell me what it is and I'll try to get it done.

Yes, please make the attached change, and rerun the script. This change
reflects the changes I made in src/tools/find_typedef.

What value is there if the URL I have just runs on Linux? It probably
has the same coverage I have for BSD. I asked for something that was
automated for more platforms a year ago and I still don't have it? I
might as well just use the same method I have have used for years.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Simon Riggs <simon@2ndQuadrant.com> writes:

On Tue, 2009-06-09 at 13:21 -0400, Bruce Momjian wrote:

It is time to run pgindent on CVS HEAD for 8.4. I am thinking of
running it at zero-hour GMT tomorrow, meaning five hours from now.

Why don't we do this automatically after each individual commit?

It's not very practical until the typedef extraction stuff is fully
automatic and fully trustworthy; right now some manual review and
intervention still seems like a good idea.

Also, that would risk breaking series of interdependent patches.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

I agree we are running out of time so I will be running pgindent in one
hour using the same BSD-based typedef method I have used in the past.
If a better typedef list appears, we can always rerun pgindent.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

Tom Lane wrote:

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

I agree we are running out of time so I will be running pgindent in one
hour using the same BSD-based typedef method I have used in the past.
If a better typedef list appears, we can always rerun pgindent.

Why not compare the BSD list with the new one and adopt anything that
seems sane? We know that your old method misses quite a lot of typedefs.

regards, tom lane

Bruce Momjian wrote:

Andrew Dunstan wrote:

Bruce Momjian wrote:

OK, Andrew, would you use the find_typedef file that is in CVS HEAD and
run that. I think that will fix our problem and then I can use the
buildfarm version. How often does that run and does it pull the script
from CVS HEAD?

The buildfarm does not run the find-typedefs script. Its code for this
is below. My Unix machine runs this once a day. I can do runs on Windows
and Cygwin manually.

If there is changed logic tell me what it is and I'll try to get it done.

Yes, please make the attached change, and rerun the script. This change
reflects the changes I made in src/tools/find_typedef.

What value is there if the URL I have just runs on Linux? It probably
has the same coverage I have for BSD. I asked for something that was
automated for more platforms a year ago and I still don't have it? I
might as well just use the same method I have have used for years.

Well, sometimes I build it and they don't come ;-).

I don't have every platform under the sun that I can run this on,
although I do now have an FBSD VM that I didn't have when I worked on
this previously. If you're actually going to use it I'll set it up as a
buildfarm member and run the find-typedefs there.

I will make the change you request and rerun the stuff on the platforms
I have that are currently buildfarm members, but AFAICT the Linux output
will still include the stat symbol (your change doesn't affect the Linux
branch at all).

Both my version and yours contains provision for filtering out certain
symbols ('timestamp' etc). Maybe we need to add 'stat' to that list.

cheers

andrew

Andrew Dunstan wrote:

Well, sometimes I build it and they don't come ;-).

I don't have every platform under the sun that I can run this on,
although I do now have an FBSD VM that I didn't have when I worked on
this previously. If you're actually going to use it I'll set it up as a
buildfarm member and run the find-typedefs there.

I will make the change you request and rerun the stuff on the platforms
I have that are currently buildfarm members, but AFAICT the Linux output
will still include the stat symbol (your change doesn't affect the Linux
branch at all).

Good point. Here is another diff I need you to make to the pl file.
If you want to make your pl file the official version and replace the
shell script in CVS, that is fine with me. Do you want me to do that?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:

Good point. Here is another diff I need you to make to the pl file.

Done. Linux run under way.

If you want to make your pl file the official version and replace the
shell script in CVS, that is fine with me. Do you want me to do that?

It needs to be done in two pieces, I think: a perl module that exports a
function that can be called from the buildfarm, and program that calls
it so it can be run standalone. The function needs two inputs: an
installation directory and a source directory. Then we will be able to
stay in sync nicely. I will try to do that over the next day or so if
you like.

Or we can just wing it for now and do this after the release.

cheers

andrew

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

I saw a few odd things. Most importantly, it seems 'stat' was
introduced as a typedef on _both_ lists, yielding weird changes like:

The standard headers do define "struct stat". I wonder whether the
objdump kluge we are using is unable to distinguish typedef names
from struct tags.

I will have to research this further tomorrow.

We don't have a lot of time for research. Maybe the best thing is to
just manually remove stat from the typedef list (along with anything
else that clearly shouldn't be there)?

The problem is that there are other symbols I don't know about and the
diff is very large. I have found that the problem was caused when we
added Linux support to find_typedef and I have a way to get an accurate
list on my machine.

OK, I have found the cause of the script error, and it was my fault. A
month after we ran pgindent for 8.3 (December 2007), I received this
issue from Tom:

http://archives.postgresql.org/pgsql-hackers/2007-12/msg00800.php

Something I noticed the other day is that pgindent doesn't seem to treat
"struct foo" or "union foo" as a type name, which is pretty silly
because no context is needed at all to recognize that. We tend not to
do that too much --- the project style is to use a typedef name --- but
there are some places that do it, particularly the regex code. For
instance there are extra spaces here:

static void
cmtreefree(struct colormap * cm,
union tree * tree,
int level) /* level number (top == 0) of this
block */
{

Fixable?

Not understanding the ramifications of adding struct and union tags to
the typedef list, I modified the BSD code:

http://archives.postgresql.org/pgsql-hackers/2007-12/msg00810.php

Yes, I found those are 't' STABS rather than "T" which are used in cases
where you do typedef struct {} name. The next pgindent will have those
typedefs you want.

and that modification was propogated to the Linux code.

This has now been fixed in the BSD and Linux code (and Perl script) and
we can move forward with running pgindent once Andrew has lists for all
the platforms he wants.

As for Tom's original complaint about, it seems BSD indent is just not
smart enough about struct/union tags. I will look into fixing that
after 8.4 pgindent is run.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Andrew Dunstan wrote:

Bruce Momjian wrote:

Good point. Here is another diff I need you to make to the pl file.

Done. Linux run under way.

If you want to make your pl file the official version and replace the
shell script in CVS, that is fine with me. Do you want me to do that?

It needs to be done in two pieces, I think: a perl module that exports a
function that can be called from the buildfarm, and program that calls
it so it can be run standalone. The function needs two inputs: an
installation directory and a source directory. Then we will be able to
stay in sync nicely. I will try to do that over the next day or so if
you like.

Or we can just wing it for now and do this after the release.

We have to wing it for now. Please let me know when whatever lists you
can generate are ready, hopefully in the next few hours.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

OK, I have found the cause of the script error, and it was my fault. A
month after we ran pgindent for 8.3 (December 2007), I received this
issue from Tom:

http://archives.postgresql.org/pgsql-hackers/2007-12/msg00800.php

Something I noticed the other day is that pgindent doesn't seem to treat
"struct foo" or "union foo" as a type name, which is pretty silly
because no context is needed at all to recognize that.

Ah. So really the point here is that we want to specifically exclude
"struct stat" because there are too many places in our code where "stat"
is used as a regular identifier. Are there any other special cases like
that?

regards, tom lane