[Bug translator/11353] elide side-effect-free probes

[Bug translator/11353] elide side-effect-free probes

[sapatel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=11353

Sagar Patel <sapatel at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sapatel at redhat dot com
           Assignee|systemtap at sourceware dot org    |sapatel at redhat dot com

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug translator/11353] elide side-effect-free probes

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2010-03-19 19:45 -------
It may be enough to delay calling derived_probe::join_group() until *after* all
of the optimization is done, at the end of semantic_pass().  This would also
take care of trickery like enable_task_finder() and need_uprobes which are done
during join_group().


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[chwang at redhat dot com]

------- Additional Comments From chwang at redhat dot com  2010-03-19 17:48 -------
I was hoping it would be possible to skip adding probes instead of deleting
them, but aside from the empty-probe case, it requires probes to be collapsed
accordingly... I'll try to find a way to at least get rid of the empty-probe
case without using a blind cast. Let me know if you think that eliding that
special case would be worth it.

:)

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2010-03-19 16:40 -------

> So the probes should be removed in semantic_pass_opt4 (where the warning is
> currently thrown)? Also, does 'removing them entirely' entail removing them from
> systemtap_session.probes? I seem to have somehow convinced myself that it does :/

Yes, but unfortunately that's not enough.  By this time in pass 2, derived_probes
are also enrolled in various derived_probe_groups.  So ones about to be elided
would also have to be 'un-enrolled'.  This could be done with an api extension
for derived_probe and derived_probe_group, and its implementation in a handful
of concrete classes.  This is sounding more and more involved....


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[chwang at redhat dot com]

------- Additional Comments From chwang at redhat dot com  2010-03-08 22:01 -------
(In reply to comment #3)
> jistone is right, casting without a full understanding of the issues
> is a recipe for trouble.
> 
> FWIW, I would probably preserve the warning about entirely elided probes
> (and suppress the messages as usual with stap -w).

Sorry about that, I had thought p->body would still be castable at that point
since it didn't look like there were any intervening steps that would change it.
The patch also misses the majority of the cases (i.e. probes that are optimized
out), and definitely breaks a bunch of tests so not too useful :(.

So the probes should be removed in semantic_pass_opt4 (where the warning is
currently thrown)? Also, does 'removing them entirely' entail removing them from
systemtap_session.probes? I seem to have somehow convinced myself that it does :/

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[dsmith at redhat dot com]

------- Additional Comments From dsmith at redhat dot com  2010-03-08 21:48 -------
(In reply to comment #6)
> (In reply to comment #5)
> > What is this going to do to the testsuite?  Lots of buildok scripts (for
> > example) do something like: 
> > 
> > probe vm.*,
> >       vm.*.* {}
> > 
> 
> Good point.
> We could add the "-t" flag to these test cases, and also make
> sure that the optimization does not apply in the -t case.

A simple solution here would be to put something in those probes, like:

probe vm.*,
      vm.*.* { printf("here\n") }



-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2010-03-08 21:17 -------
(In reply to comment #5)
> What is this going to do to the testsuite?  Lots of buildok scripts (for
> example) do something like: 
> 
> probe vm.*,
>       vm.*.* {}
> 

Good point.
We could add the "-t" flag to these test cases, and also make
sure that the optimization does not apply in the -t case.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[dsmith at redhat dot com]

------- Additional Comments From dsmith at redhat dot com  2010-03-08 21:14 -------
What is this going to do to the testsuite?  Lots of buildok scripts (for
example) do something like: 

probe vm.*,
      vm.*.* {}


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2010-03-08 21:07 -------
(In reply to comment #2)
> Although we also need to decide what should be done if the entire script has no
> useful probes -- that should probably be an outright error.

I notice that the current "no probes" error is after all the optimization.  So
having a warning on eliding-empty-probe, followed by the existing "semantic
error: no probes found", I think is fine.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2010-03-08 20:47 -------
jistone is right, casting without a full understanding of the issues
is a recipe for trouble.

FWIW, I would probably preserve the warning about entirely elided probes
(and suppress the messages as usual with stap -w).

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2010-03-08 20:38 -------
(In reply to comment #1)
> Skips empty probes by checking if p->body->statements.size() == 0. I had to do
> a cast to (block*), it seems like it should cast all right since
> parse_stmt_block() returns a block*. (p->body = parse_stmt_block(), 2 lines
> above changes)

If that were really guaranteed, then we should make ->body a block* -- but it's
not the case.  Some of the later optimization passes will collapse singleton
blocks.  In the extreme case, a probe can get optimized completely away, in
which it becomes just a null_statement.

At the place where the warning is actually thrown, you can see the
null_statement replacement happening -- I think that's where you should take action.

> Not sure if this is what you want, or if you just want to get rid of the
> warning message in elaborate.cxx?

I think the goal is to get rid of warning but also to not even register the
empty probe, so it incurs no overhead.

> Rudimentary tests (e.g. stap -e 'probe begin {printf("HAI") exit()} probe end
> {}') seem to work.

Something like this should be part of your test:
  probe begin { if (@defined($foo)) println("WUT?") }

Although we also need to decide what should be done if the entire script has no
useful probes -- that should probably be an outright error.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jistone at redhat dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug translator/11353] elide side-effect-free probes

[chwang at redhat dot com]

------- Additional Comments From chwang at redhat dot com  2010-03-08 20:17 -------
Created an attachment (id=4654)
 --> (http://sourceware.org/bugzilla/attachment.cgi?id=4654&action=view)
Patch for 11353

Skips empty probes by checking if p->body->statements.size() == 0. I had to do
a cast to (block*), it seems like it should cast all right since
parse_stmt_block() returns a block*. (p->body = parse_stmt_block(), 2 lines
above changes)

Not sure if this is what you want, or if you just want to get rid of the
warning message in elaborate.cxx?

Rudimentary tests (e.g. stap -e 'probe begin {printf("HAI") exit()} probe end
{}') seem to work.

Running make installcheck, since I'm not sure that this won't break anything
else :/

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=11353

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.