Bug 488941
Summary: | Enable static marker support for systemtap | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Mark Wielaard <mjw> | ||||
Component: | postgresql | Assignee: | Tom Lane <tgl> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | low | ||||||
Version: | rawhide | CC: | fche, hhorak, scox, tao, tgl, wcohen | ||||
Target Milestone: | --- | ||||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2009-03-08 21:03:19 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 546295 | ||||||
Attachments: |
|
Description
Mark Wielaard
2009-03-06 11:02:59 UTC
Created attachment 334284 [details]
postgres spec patch to enable sdt
I'd be happier about turning this on if it didn't mean that the package would fail to build on my F-10 development machine. Let alone any question of testing it :-(. What is the situation for systemtap-sdt-devel support on F-10? (In reply to comment #2) > What is the situation for systemtap-sdt-devel support on F-10? It is part of systemtap 0.9, which is in updates-testing at the moment: https://admin.fedoraproject.org/updates/F10/FEDORA-2009-2055 The examples and testing mentioned in the description were all done on F10 with that package installed. ok, got it. BTW, I did "yum install systemtap-sdt-devel" and it happily pulled down just that package, without updating systemtap or systemtap-runtime. Shouldn't there be an intrapackage Requires: to force all three to be the same version? (In reply to comment #4) > ok, got it. BTW, I did "yum install systemtap-sdt-devel" and it happily pulled > down just that package, without updating systemtap or systemtap-runtime. > Shouldn't there be an intrapackage Requires: to force all three to be the same > version? If we did things right then the systemtap runtime works against any version of systemtap-sdt-devel a package has been compiled against. So there is no hard dependency between de sdt-devel and runtime package versions. That said it might be confusing to the user to have the versions of these packages be different (the 0.8 version of the runtime doesn't support user space static markers). I'll ask our systemtap packaging gurus (by adding them to the CC). Tom, it is not our desire to require that an instrumented program such as postgres *require* the installation of the management tool. Instrumented postgres will continue to run just fine without the monitoring active. No, you miss my point. If I choose to install systemtap-sdt-devel 0.9, I would expect the packaging system to make sure that the underlying systemtap package is compatible with it. I'm not expecting surprises like the devel stuff will accept userspace markers and then the underlying fails to work with them. It's entirely laudable that I don't have to recompile any existing stuff when updating systemtap, but that's not really the issue here. I understand the concern, but I can't think of a trivial rpm recipe to express that. For one thing, the -sdt-devel is only a buildreq and not a requires, so it does not need to be installed on an end-user machine in order to use the instrumentation at run time. Maybe we could adjust the postgres spec file to conflict with a virtual systemtap provide:, as in: systemtap.spec: provide: systemtap-sdt-abi = 0.9 postgresql.spec: conflict: systemtap-sdt-abi < 0.9 We're still not communicating: this is entirely unrelated to Postgres. What I'm expecting is that the systemtap-sdt-devel package ought to carry Requires: systemtap = %{version}-%{release} so that you could not install a systemtap-sdt-devel package that's inconsistent with the runtime support. But anyway, back to the original request. I'm testing this out here and I notice that every probe point draws a gcc warning: lwlock.c: In function 'LWLockAcquire': lwlock.c:450: warning: ISO C90 forbids mixed declarations and code lwlock.c:461: warning: ISO C90 forbids mixed declarations and code lwlock.c:472: warning: ISO C90 forbids mixed declarations and code lwlock.c: In function 'LWLockConditionalAcquire': lwlock.c:543: warning: ISO C90 forbids mixed declarations and code lwlock.c:549: warning: ISO C90 forbids mixed declarations and code lwlock.c: In function 'LWLockRelease': lwlock.c:634: warning: ISO C90 forbids mixed declarations and code That's *highly* annoying. Is there a plan for getting rid of that? We're still not communicating: this is entirely unrelated to Postgres. What I'm expecting is that the systemtap-sdt-devel package ought to carry Requires: systemtap = %{version}-%{release} so that you could not install a systemtap-sdt-devel package that's inconsistent with the runtime support. But that would accomplish nothing. Systemtap does not use the -sdt-devel package at run time at all, and vice versa. Thus no install or build-time require is appropriate in either direction. What matters is that the instrumented package (such as postgres or whatnot) that *built* against a particular -sdt-devel should be *run* against a compatible systemtap. I understand that you've carefully made the stuff able to work across versions. What I'm talking about is the expectation of a Fedora user that he doesn't get subpackages that are wildly inconsistent with each other capability-wise. There's a packaging guideline that spells this out: https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package That guideline isn't about whether it's technically possible to mix-and-match, it's a policy decision that says RPM should keep subpackages in sync automatically. Hm, I just ran into a bigger problem: it doesn't work at all against Postgres CVS HEAD. /home/tgl/pgsql/src/backend/storage/buffer/bufmgr.c:439: undefined reference to `TRACE_POSTGRESQL_BUFFER_READ_DONE' storage/smgr/md.o: In function `mdwrite': /home/tgl/pgsql/src/backend/storage/smgr/md.c:663: undefined reference to `TRACE_POSTGRESQL_SMGR_MD_WRITE_DONE' storage/smgr/md.o: In function `mdread': /home/tgl/pgsql/src/backend/storage/smgr/md.c:599: undefined reference to `TRACE_POSTGRESQL_SMGR_MD_READ_DONE' collect2: ld returned 1 exit status I find it suspicious that these are the only probes with more than six arguments (they have seven or eight). sys/sdt.h seems to be prepared to support up to nine arguments but I wonder whether dtrace itself knows that. The generated probes.h file simply fails to define the expected macros at all, though it does contain the corresponding _ENABLED macros. Ring a bell anyone? (In reply to comment #10) > But anyway, back to the original request. I'm testing this out here and I > notice that every probe point draws a gcc warning: > > lwlock.c: In function 'LWLockAcquire': > lwlock.c:450: warning: ISO C90 forbids mixed declarations and code > lwlock.c:461: warning: ISO C90 forbids mixed declarations and code > lwlock.c:472: warning: ISO C90 forbids mixed declarations and code > lwlock.c: In function 'LWLockConditionalAcquire': > lwlock.c:543: warning: ISO C90 forbids mixed declarations and code > lwlock.c:549: warning: ISO C90 forbids mixed declarations and code > lwlock.c: In function 'LWLockRelease': > lwlock.c:634: warning: ISO C90 forbids mixed declarations and code > > That's *highly* annoying. Is there a plan for getting rid of that? I added Stan, who does all the sdt.h magic, to the CC. Although it seems this particular issue has been fixed in our recent git version. That version isn't compatible with -ansi. (In reply to comment #13) > I find it suspicious that these are the only probes with more than six > arguments (they have seven or eight). sys/sdt.h seems to be prepared to > support up to nine arguments but I wonder whether dtrace itself knows that. > The generated probes.h file simply fails to define the expected macros at all, > though it does contain > the corresponding _ENABLED macros. Ring a bell anyone? Yes. This was an issue against icedtea/hotspot also, since fixed in systemtap git. Seems we need an updated systemtap-sdt-devel then. > What I'm talking about is the expectation of a Fedora user that he doesn't get
> subpackages that are wildly inconsistent with each other capability-wise.
> There's a packaging guideline that spells this out:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
This seems most appropriate for typical -devel packages that are not actually
self-contained - ones that can't be used without the run-time component
also being installed. Several -devel packages for example seem to miss the
actual .so they're linking against. That's not the case here.
Nor is it the case that something built with -sdt-devel-NNN will fail to "work"
in the way that it would with normal foo and foo-devel packages, where the newly
built/linked program would be unable to even start. Here, at worst some probes
would be unavailable to a systemtap user, but otherwise the instrumented program
- and even the rest of systemtap - would run fine.
Anyway, this part is not a very strong argument either way, nor as you point out,
relevant/specific to postgres. Perhaps we could gather advice on fedora-devel.
> Perhaps we could gather advice on fedora-devel.
Agreed; this seems to be a judgment call, so this bugzilla isn't the place to resolve it.
Will the other issues be resolved soon? I am eager to play with systemtap against Postgres HEAD.
In the meantime, I've pushed the requested Postgres patch into rawhide, since there's little time left before beta freeze. (In reply to comment #17) > Will the other issues be resolved soon? I am eager to play with systemtap > against Postgres HEAD. I think I got fixes for the issues/warnings mentioned. -ansi -pedantic isn't going to happen though. What warning flags are you using? Not -pedantic ;-). AFAICT it's -Wdeclaration-after-statement that's producing this. A typical compile command in the RPM build looks like gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wall -Wmissing-prototypes -Wpointer-arith -Winline -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -I../../../../src/include -D_GNU_SOURCE -I/usr/include/libxml2 -c -o lwlock.o lwlock.c > Agreed; this seems to be a judgment call, so this bugzilla isn't the place to > resolve it. Thanks for bringing it up. > Will the other issues be resolved soon? I am eager to play with systemtap > against Postgres HEAD. We will respin a new systemtap very soon, but in the mean time building it out of git for yourself is pretty easy. Install elfutils-devel, git clone git://sources.redhat.com/git/systemtap.git configure --prefix=SOMEWHERE make all install (and you don't need the static marker stuff necessarily; you can probe apprx. any function/statement in your program.) (In reply to comment #21) > > Will the other issues be resolved soon? I am eager to play with systemtap > > against Postgres HEAD. > > We will respin a new systemtap very soon, but in the mean time building > it out of git for yourself is pretty easy. Install elfutils-devel, > git clone git://sources.redhat.com/git/systemtap.git > configure --prefix=SOMEWHERE > make all install I pushed all sdt.h cleanups I had based on your comments into upstream systemtap git this morning. If you can please do try it out and give us any feedback against Postgres HEAD. Thanks for your quick responses and feedback. Unfortunately, this morning's upstream git seems broken too, only differently. The "ISO C90 forbids mixed declarations and code" warnings are still there, and I'm now getting gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -fwrapv -g -I../../../../src/include -D_GNU_SOURCE -I/home/tgl/my-systemtap/include -c -o lock.o lock.c lock.c: In function 'LockAcquire': lock.c:790: error: assignment of read-only variable 'arg1' lock.c:794: error: assignment of read-only variable 'arg1' make: *** [lock.o] Error 1 This is coming from macros like TRACE_POSTGRESQL_LOCK_WAIT_DONE(locktag->locktag_field2, lockmode); where "locktag" is declared "const LOCKTAG *locktag". I wouldn't have thought typeof() would propagate const-ness but it seems it does. The good news is that dtrace is correctly emitting 7- and 8-argument macros, so at least we have some forward progress. After looking at what the trace macros expand to, I'm kinda confused where the mixed-decls warning is coming from. However, might I suggest that it would be safer if asm, volatile, etc were spelled __asm__, __volatile__, etc? FYI. systemtap 0.9.5 was released with a host of sdt.h cleanups. It is already in rawhide and has a pending update for f10: http://sourceware.org/ml/systemtap/2009-q1/msg00951.html https://admin.fedoraproject.org/updates/systemtap-0.9.5-1.fc10 |