bigsql / plprofiler

Other
79 stars 22 forks source link

Not compatible with 32-bit: error: lvalue required as unary ‘&’ operand #13

Closed df7cb closed 1 year ago

df7cb commented 1 year ago

While building the Debian package for plprofiler I noticed it's failing on all 32-bit distributions we support:

16:09:11 ### PostgreSQL 15 build ###
16:09:11 make[2]: Entering directory '/<<PKGBUILDDIR>>/build-15'
16:09:11 gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -I. -I/<<PKGBUILDDIR>> -I/usr/include/postgresql/15/server -I/usr/include/postgresql/internal  -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2   -c -o plprofiler.o /<<PKGBUILDDIR>>/plprofiler.c
16:09:11 In file included from /<<PKGBUILDDIR>>/plprofiler.c:20:
16:09:11 /<<PKGBUILDDIR>>/plprofiler.c: In function ‘pl_profiler_funcs_source’:
16:09:11 /usr/include/postgresql/15/server/postgres.h:804:47: error: lvalue required as unary ‘&’ operand
16:09:11   804 | #define Int64GetDatumFast(X)  PointerGetDatum(&(X))
16:09:11       |                                               ^
16:09:11 /usr/include/postgresql/15/server/postgres.h:600:38: note: in definition of macro ‘PointerGetDatum’
16:09:11   600 | #define PointerGetDatum(X) ((Datum) (X))
16:09:11       |                                      ^
16:09:11 /<<PKGBUILDDIR>>/plprofiler.c:1829:39: note: in expansion of macro ‘Int64GetDatumFast’
16:09:11  1829 |                         values[i++] = Int64GetDatumFast(line_number++);
16:09:11       |                                       ^~~~~~~~~~~~~~~~~
16:09:11 make[2]: *** [<builtin>: plprofiler.o] Error 1

Full log: https://pgdgbuild.dus.dg-i.net/job/plprofiler-binaries/2/architecture=i386,distribution=sid/console

Is that intended?

luss commented 1 year ago

Do you have a suggested fix? Jan probably can, but, I don't personally do thirty two bit anymore. Although, 32 bit debian is still the default OS on the Raspberry PI!!

On Fri, Nov 18, 2022 at 10:13 AM Christoph Berg @.***> wrote:

While building the Debian package for plprofiler I noticed it's failing on all 32-bit distributions we support:

16:09:11 ### PostgreSQL 15 build ###

16:09:11 make[2]: Entering directory '/<>/build-15'

16:09:11 gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -ffile-prefix-map=/<>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -I. -I/<> -I/usr/include/postgresql/15/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o plprofiler.o /<>/plprofiler.c

16:09:11 In file included from /<>/plprofiler.c:20:

16:09:11 /<>/plprofiler.c: In function ‘pl_profiler_funcs_source’:

16:09:11 /usr/include/postgresql/15/server/postgres.h:804:47: error: lvalue required as unary ‘&’ operand

16:09:11 804 | #define Int64GetDatumFast(X) PointerGetDatum(&(X))

16:09:11 | ^

16:09:11 /usr/include/postgresql/15/server/postgres.h:600:38: note: in definition of macro ‘PointerGetDatum’

16:09:11 600 | #define PointerGetDatum(X) ((Datum) (X))

16:09:11 | ^

16:09:11 /<>/plprofiler.c:1829:39: note: in expansion of macro ‘Int64GetDatumFast’

16:09:11 1829 | values[i++] = Int64GetDatumFast(line_number++);

16:09:11 | ^~~~~

16:09:11 make[2]: *** [: plprofiler.o] Error 1

Full log: https://pgdgbuild.dus.dg-i.net/job/plprofiler-binaries/2/architecture=i386,distribution=sid/console

Is that intended?

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMWOHRH4IQEDZLUVZ4YKF3WI6MIBANCNFSM6AAAAAASES5B6U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

wieck commented 1 year ago

On 11/18/22 10:17, Denis Lussier wrote:

Do you have a suggested fix? Jan probably can, but, I don't personally do thirty two bit anymore. Although, 32 bit debian is still the default OS on the Raspberry PI!!

The fix would probably be to generate the function source code line number from a 32 bit integer. I'll have to look what other side effects that might have. 64 bits for that is overkill anyway, since I believe a PL/pgSQL function with more than 2 billion lines of source code will produce other problems.

Best Regards, Jan

On Fri, Nov 18, 2022 at 10:13 AM Christoph Berg @.***> wrote:

While building the Debian package for plprofiler I noticed it's failing on all 32-bit distributions we support:

16:09:11 ### PostgreSQL 15 build ###

16:09:11 make[2]: Entering directory '/<>/build-15'

16:09:11 gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -ffile-prefix-map=/<>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -I. -I/<> -I/usr/include/postgresql/15/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o plprofiler.o /<>/plprofiler.c

16:09:11 In file included from /<>/plprofiler.c:20:

16:09:11 /<>/plprofiler.c: In function ‘pl_profiler_funcs_source’:

16:09:11 /usr/include/postgresql/15/server/postgres.h:804:47: error: lvalue required as unary ‘&’ operand

16:09:11 804 | #define Int64GetDatumFast(X) PointerGetDatum(&(X))

16:09:11 | ^

16:09:11 /usr/include/postgresql/15/server/postgres.h:600:38: note: in definition of macro ‘PointerGetDatum’

16:09:11 600 | #define PointerGetDatum(X) ((Datum) (X))

16:09:11 | ^

16:09:11 /<>/plprofiler.c:1829:39: note: in expansion of macro ‘Int64GetDatumFast’

16:09:11 1829 | values[i++] = Int64GetDatumFast(line_number++);

16:09:11 | ^~~~~

16:09:11 make[2]: *** [: plprofiler.o] Error 1

Full log:

https://pgdgbuild.dus.dg-i.net/job/plprofiler-binaries/2/architecture=i386,distribution=sid/console

Is that intended?

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13, or unsubscribe

https://github.com/notifications/unsubscribe-auth/AAMWOHRH4IQEDZLUVZ4YKF3WI6MIBANCNFSM6AAAAAASES5B6U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13#issuecomment-1320164210, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACYRMQIWCS6JMORIEF4W5LWI6MZ3ANCNFSM6AAAAAASES5B6U. You are receiving this because you are subscribed to this thread.Message ID: @.***>

luss commented 1 year ago

I'm comfortable with a 2 billion line max number of source lines in a plsql function/proc. :-). Even if that is for all of the functions that may be loaded in a session. :-)

On Fri, Nov 18, 2022 at 10:33 AM Jan Wieck @.***> wrote:

On 11/18/22 10:17, Denis Lussier wrote:

Do you have a suggested fix? Jan probably can, but, I don't personally do thirty two bit anymore. Although, 32 bit debian is still the default OS on the Raspberry PI!!

The fix would probably be to generate the function source code line number from a 32 bit integer. I'll have to look what other side effects that might have. 64 bits for that is overkill anyway, since I believe a PL/pgSQL function with more than 2 billion lines of source code will produce other problems.

Best Regards, Jan

On Fri, Nov 18, 2022 at 10:13 AM Christoph Berg @.***> wrote:

While building the Debian package for plprofiler I noticed it's failing on all 32-bit distributions we support:

16:09:11 ### PostgreSQL 15 build ###

16:09:11 make[2]: Entering directory '/<>/build-15'

16:09:11 gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -g -O2 -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -ffile-prefix-map=/<>=. -fstack-protector-strong -Wformat -Werror=format-security -fPIC -I. -I/<> -I/usr/include/postgresql/15/server -I/usr/include/postgresql/internal -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c -o plprofiler.o /<>/plprofiler.c

16:09:11 In file included from /<>/plprofiler.c:20:

16:09:11 /<>/plprofiler.c: In function ‘pl_profiler_funcs_source’:

16:09:11 /usr/include/postgresql/15/server/postgres.h:804:47: error: lvalue required as unary ‘&’ operand

16:09:11 804 | #define Int64GetDatumFast(X) PointerGetDatum(&(X))

16:09:11 | ^

16:09:11 /usr/include/postgresql/15/server/postgres.h:600:38: note: in definition of macro ‘PointerGetDatum’

16:09:11 600 | #define PointerGetDatum(X) ((Datum) (X))

16:09:11 | ^

16:09:11 /<>/plprofiler.c:1829:39: note: in expansion of macro ‘Int64GetDatumFast’

16:09:11 1829 | values[i++] = Int64GetDatumFast(line_number++);

16:09:11 | ^~~~~

16:09:11 make[2]: *** [: plprofiler.o] Error 1

Full log:

https://pgdgbuild.dus.dg-i.net/job/plprofiler-binaries/2/architecture=i386,distribution=sid/console

Is that intended?

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13, or unsubscribe

< https://github.com/notifications/unsubscribe-auth/AAMWOHRH4IQEDZLUVZ4YKF3WI6MIBANCNFSM6AAAAAASES5B6U

. You are receiving this because you are subscribed to this thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13#issuecomment-1320164210, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AACYRMQIWCS6JMORIEF4W5LWI6MZ3ANCNFSM6AAAAAASES5B6U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13#issuecomment-1320183634, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMWOHTKMQPSTQG7FF3HMBTWI6OT5ANCNFSM6AAAAAASES5B6U . You are receiving this because you commented.Message ID: @.***>

df7cb commented 1 year ago

The total number of calls to a function might easily exceed 2^32, though, in case "line count" isn't the only number involved there.

wieck commented 1 year ago

On 11/19/22 07:20, Christoph Berg wrote:

The total number of calls to a function might easily exceed 2^32, though, in case "line count" isn't the only number involved there.

Looking at the code in line number 1829 it is the only occurrence of a combination of the Int64GetDatumFast() macro with a ++ increment of the value. There are many more uses of Int64GetDatumFast() in the code, which I would expect to work.

I'll see if I can reproduce the error and if moving the increment onto a separate code line fixes the problem.

Thank you for your time, Jan

wieck commented 1 year ago

On 11/20/22 08:43, Jan Wieck wrote:

On 11/19/22 07:20, Christoph Berg wrote:

The total number of calls to a function might easily exceed 2^32, though, in case "line count" isn't the only number involved there.

Looking at the code in line number 1829 it is the only occurrence of a combination of the Int64GetDatumFast() macro with a ++ increment of the value. There are many more uses of Int64GetDatumFast() in the code, which I would expect to work.

I'll see if I can reproduce the error and if moving the increment onto a separate code line fixes the problem.

There were actually more problems. Making the ++ a separate command fixed the compiler error, but then the extension actually crashed the backend when using it.

The problem for that was that a literal '0' becomes an int, which now defaults to int32, and casting that to (Datum) can produce a SIGSEGV because the datum is not properly aligned.

I have fixed both issues and pushed them into test branch

https://github.com/bigsql/plprofiler/tree/fix_for_32bit_debian

I did test all this on debian-11.5-i386 as well as Rocky 8.6 x86_64. Does anyone else want to perform some testing before I merge and tag this as REL4_2_1?

Best Regards, Jan

df7cb commented 1 year ago

Thanks, that fixes the problem here!

wieck commented 1 year ago

On 11/22/22 04:01, Christoph Berg wrote:

Thanks, that fixes the problem here!

I merged the fix and tagged REL4_2_1 as the new 4.2.1 release.

Thank you, Jan

— Reply to this email directly, view it on GitHub https://github.com/bigsql/plprofiler/issues/13#issuecomment-1323326280, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACYRMVVVLAYGHK2YR3OHB3WJSDWPANCNFSM6AAAAAASES5B6U. You are receiving this because you commented.Message ID: @.***>

df7cb commented 1 year ago

Thanks for the fix! I finished the Debian packages and uploaded them to apt.postgresql.org and Debian.