Bug 488941 - Enable static marker support for systemtap
Enable static marker support for systemtap
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: postgresql (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Tom Lane
Fedora Extras Quality Assurance
:
Depends On:
Blocks: StapStaticProbesF13
  Show dependency treegraph
 
Reported: 2009-03-06 06:02 EST by Mark Wielaard
Modified: 2013-07-02 23:21 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-08 17:03:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
postgres spec patch to enable sdt (1.02 KB, patch)
2009-03-06 06:03 EST, Mark Wielaard
no flags Details | Diff

  None (edit)
Description Mark Wielaard 2009-03-06 06:02:59 EST
Please consider adding static marker support for Systemtap. This is part of the Fedora Systemtap Static Probes effort: https://fedoraproject.org/wiki/Features/SystemtapStaticProbes

Some examples of what is possible when enabling this can be seen in this blog post:
http://gnu.wildebeest.org/diary/2009/02/24/systemtap-09-markers-everywhere/

And the following video:
http://people.redhat.com/wcohen/postgresql_example.ogv

Attached is a patch for the postgres.spec file.
Comment 1 Mark Wielaard 2009-03-06 06:03:57 EST
Created attachment 334284 [details]
postgres spec patch to enable sdt
Comment 2 Tom Lane 2009-03-07 23:29:00 EST
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?
Comment 3 Mark Wielaard 2009-03-08 08:03:06 EDT
(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.
Comment 4 Tom Lane 2009-03-08 13:10:22 EDT
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?
Comment 5 Mark Wielaard 2009-03-08 13:34:47 EDT
(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).
Comment 6 Frank Ch. Eigler 2009-03-08 13:56:51 EDT
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.
Comment 7 Tom Lane 2009-03-08 14:23:05 EDT
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.
Comment 8 Frank Ch. Eigler 2009-03-08 14:45:10 EDT
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
Comment 9 Tom Lane 2009-03-08 14:57:23 EDT
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.
Comment 10 Tom Lane 2009-03-08 14:59:09 EDT
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?
Comment 11 Frank Ch. Eigler 2009-03-08 15:09:12 EDT
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.
Comment 12 Tom Lane 2009-03-08 15:20:42 EDT
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.
Comment 13 Tom Lane 2009-03-08 15:30:46 EDT
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?
Comment 14 Mark Wielaard 2009-03-08 15:38:12 EDT
(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.
Comment 15 Mark Wielaard 2009-03-08 15:40:14 EDT
(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.
Comment 16 Frank Ch. Eigler 2009-03-08 15:52:42 EDT
> 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.
Comment 17 Tom Lane 2009-03-08 16:03:37 EDT
> 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.
Comment 18 Tom Lane 2009-03-08 17:03:19 EDT
In the meantime, I've pushed the requested Postgres patch into rawhide, since there's little time left before beta freeze.
Comment 19 Mark Wielaard 2009-03-08 19:02:43 EDT
(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?
Comment 20 Tom Lane 2009-03-08 19:19:01 EDT
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
Comment 21 Frank Ch. Eigler 2009-03-08 19:30:53 EDT
> 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.)
Comment 22 Mark Wielaard 2009-03-09 07:14:20 EDT
(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.
Comment 23 Tom Lane 2009-03-09 14:05:30 EDT
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?
Comment 24 Mark Wielaard 2009-03-30 05:23:46 EDT
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

Note You need to log in before you can comment on or make changes to this bug.