[Bug runtime/2497] New: STP_STRING_SIZE set by stap is too small

[Bug runtime/2497] New: STP_STRING_SIZE set by stap is too small

[guanglei at cn dot ibm dot com]

Hi,
 If you use:
   log(backtrace( )) 
 in your codes, you will find that in many cases the result string will be
truncated.
   
  I found it is due to the following codes generated by translator:
   ...
   #ifndef MAXSTRINGLEN
   #define MAXSTRINGLEN 128
   ...
   #define STP_STRING_SIZE MAXSTRINGLEN

  so STP_STRING_SIZE is set to 128, and this is obviously too small for some
underlying transport functions.

-- 
           Summary: STP_STRING_SIZE set by stap is too small
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: runtime
        AssignedTo: systemtap at sources dot redhat dot com
        ReportedBy: guanglei at cn dot ibm dot com


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-03-30 11:45 -------
Any particular number constitutes a trade-off.  What number would you propose,
keeping in mind that all string instances (scalars, temporaries, array
keys/values) are statically allocated and copied by value?

The problem of long backtraces is already to some extent addressed by the
print_backtrace() function.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[jrs at us dot ibm dot com]

------- Additional Comments From jrs at us dot ibm dot com  2006-03-30 14:29 -------
When I use backtrace(),  I usually run stap with -DMAXSTRINGLEN=256.  This seems
to cover back traces on PPC64 very well.  As for changing the default, why would
the default of  MAXSTRINGLEN=128 need to change if can be tune this way? 

-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[bibo dot mao at intel dot com]

------- Additional Comments From bibo dot mao at intel dot com  2006-03-31 02:50 -------
When test NFS performance in i386, even run stap with -DMAXSTRINGLEN=256, 
sometimes it can not print all the back trace history.  I doubt that stack will 
override if trap reentrance happens and CONFIG_4KSTACKS option is set. 

-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[guanglei at cn dot ibm dot com]

------- Additional Comments From guanglei at cn dot ibm dot com  2006-03-31 13:35 -------
(In reply to comment #2)
> When I use backtrace(),  I usually run stap with -DMAXSTRINGLEN=256.  This seems
> to cover back traces on PPC64 very well.  As for changing the default, why would
> the default of  MAXSTRINGLEN=128 need to change if can be tune this way? 
It seems not a good idea to simplily increase MAXSTRINGLEN, which will cause a
lot of unecessary additional memory consumption.

MAXSTRINGLEN =128 is ok. But I think we should slightly change the translator:

-bash-3.00# diff -uprN translate.cxx.ori translate.cxx
--- translate.cxx.ori   2006-03-31 13:26:20.000000000 -0500
+++ translate.cxx       2006-03-31 13:21:29.000000000 -0500
@@ -3774,7 +3774,9 @@ translate_pass (systemtap_session& s)
       s.op->newline() << "#endif";

       // impedance mismatch
+      s.op->newline() << "#ifndef STP_STRING_SIZE";
       s.op->newline() << "#define STP_STRING_SIZE MAXSTRINGLEN";
+      s.op->newline() << "#endif";
       s.op->newline() << "#define STP_NUM_STRINGS 1";

       if (s.bulk_mode)

Increasing STP_STRING_SIZE by -D will cause the size of "struct string" defined
in runtime/string.h to increase, while keep the size of other types of strings
unchanged.

So now if I want to print backtrace or want some large _stp_string inside my
codes, I can:
stap -D STP_STRING_SIZE=2048 ...
I tested it and it worked well for me.

-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2006-03-31 14:35 -------
Instances of the runtime's _stp_string type are not integrated into the
translator / scripting system.  Use of this type will cause truncation anyway as
soon as such "string" values are processed in an expression involving ordinary
strings.

While I am not opposed to the specific change you suggest, I believe it is an
inadequate and fragile solution to the problem.


-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[guanglei at cn dot ibm dot com]

------- Additional Comments From guanglei at cn dot ibm dot com  2006-03-31 14:49 -------
> So now if I want to print backtrace or want some large _stp_string inside my
> codes, I can:
> stap -D STP_STRING_SIZE=2048 ...
> I tested it and it worked well for me.

sorry, forgot to mention  that the 'it' refers to LKET.

backtrace() will still be truncated because of the limitations of its return
value(type of string_t).

So if a use want the complete backtrace to be stored in a large string buffer,
he can use the embedded c code.


-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-03-31 17:49 -------
If you use backtrace(), it puts it's results in a string, so MAXSTRINGLEN must
be adjusted.

If you use print_backtrace() it writes the output directly to stdout so there is
no limit. EXCEPT I see the ppc64 __stp_stack_sprint() function has put an
incorrect limit there. Perhaps that is the real problem you are trying to fix?

If you want to fix that function, consider that if "str == _stp_stdout" then the
output is going to the output buffer, not a string of size STP_STRING_SIZE. Also
  it looks like this backtrace function loops until it has filled up the output
buffer, not finished the backtrace, which is a very bad idea, especially because
it is not clear to me this won't loop forever in some cases.








-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[guanglei at cn dot ibm dot com]

------- Additional Comments From guanglei at cn dot ibm dot com  2006-04-03 03:43 -------
(In reply to comment #7)
> 
> If you use print_backtrace() it writes the output directly to stdout so there is
> no limit. EXCEPT I see the ppc64 __stp_stack_sprint() function has put an
> incorrect limit there. Perhaps that is the real problem you are trying to fix?
> 
> If you want to fix that function, consider that if "str == _stp_stdout" then the
> output is going to the output buffer, not a string of size STP_STRING_SIZE. 
> 
Up to now, print_backtrace() works well for me and will give a complete backtrace.

I can call print_backtrace() to print the backtrace, but in fact I want to
combine the string of backtrace with other information to generate the output of
a specific event, like the following:

String str = _stp_string_init (0);
_stp_stack_sprint (str, CONTEXT->regs, 0);
_lket_trace(THIS->var_id, "%1b%4b%1b%ld%4b%4b%s", cmd->device->sdev_state,
scsi_info,
                cmd->sc_data_direction, cmd->request_buffer, cmd->request_bufflen,
                cmd->device->host->cmd_pid, _stp_string_ptr(str));

So what I want is a larger String obtained by _stp_string_init, while keep all
other strings used by stap the same as before.

-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-04-04 00:38 -------
Subject: Re:  STP_STRING_SIZE set by stap is too small

On Mon, 2006-04-03 at 03:43 +0000, guanglei at cn dot ibm dot com wrote:
> I can call print_backtrace() to print the backtrace, but in fact I
> want to
> combine the string of backtrace with other information to generate the
> output of
> a specific event, like the following:
> 
> String str = _stp_string_init (0);
> _stp_stack_sprint (str, CONTEXT->regs, 0);
> _lket_trace(THIS->var_id, "%1b%4b%1b%ld%4b%4b%s",
> cmd->device->sdev_state,
> scsi_info,
>                 cmd->sc_data_direction, cmd->request_buffer,
> cmd->request_bufflen,
>                 cmd->device->host->cmd_pid, _stp_string_ptr(str));
> 
> So what I want is a larger String obtained by _stp_string_init, while
> keep all other strings used by stap the same as before. 

But aren't there just two possibilities?
1. You print the backtrace.
2. You get the backtrace in a string and save it in an array.

For #1, you use print_backtrace. For #2, you need to have all strings be
larger, as it is done now.

In your example above, you would just write something like this in
script:

bin_write("%1b%4b%1b%ld%4b%4b", cmd->device->sdev_state, [...])
print_backtrace()

Martin



-- 


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

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

Re: [Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[Martin Hunt]

On Mon, 2006-04-03 at 03:43 +0000, guanglei at cn dot ibm dot com wrote:
> I can call print_backtrace() to print the backtrace, but in fact I
> want to
> combine the string of backtrace with other information to generate the
> output of
> a specific event, like the following:
> 
> String str = _stp_string_init (0);
> _stp_stack_sprint (str, CONTEXT->regs, 0);
> _lket_trace(THIS->var_id, "%1b%4b%1b%ld%4b%4b%s",
> cmd->device->sdev_state,
> scsi_info,
>                 cmd->sc_data_direction, cmd->request_buffer,
> cmd->request_bufflen,
>                 cmd->device->host->cmd_pid, _stp_string_ptr(str));
> 
> So what I want is a larger String obtained by _stp_string_init, while
> keep all other strings used by stap the same as before. 

But aren't there just two possibilities?
1. You print the backtrace.
2. You get the backtrace in a string and save it in an array.

For #1, you use print_backtrace. For #2, you need to have all strings be
larger, as it is done now.

In your example above, you would just write something like this in
script:

bin_write("%1b%4b%1b%ld%4b%4b", cmd->device->sdev_state, [...])
print_backtrace()

Martin

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[guij at cn dot ibm dot com]

------- Additional Comments From guij at cn dot ibm dot com  2006-04-04 02:08 -------
(In reply to comment #9)
> But aren't there just two possibilities?
> 1. You print the backtrace.
> 2. You get the backtrace in a string and save it in an array.
> 
> For #1, you use print_backtrace. For #2, you need to have all strings be
> larger, as it is done now.
> 
> In your example above, you would just write something like this in
> script:
> 
> bin_write("%1b%4b%1b%ld%4b%4b", cmd->device->sdev_state, [...])
> print_backtrace()
> 
I think _lket_trace() is more like:
bin_write("%n%1b%4b%1b%ld%4b%4b%s",this_event_len(), cmd->device->sdev_state,
[...], _stp_string_ptr(str))
The total length of the trace record (including backtrace string) is to be
calculated and filled by bin_write(), thus only #2 can work.


-- 


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

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

Re: [Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[Frank Ch. Eigler]

Hi -

hunt wrote:

> [...]
> bin_write("%1b%4b%1b%ld%4b%4b", cmd->device->sdev_state, [...])
> print_backtrace()

It's not at all your fault, but these backtrace-related examples make
binary tracing look almost silly, in terms of performance and
complexity.

It would not be a big improvement, but the following would enable
something you suggested long ago: support a printf formatting code for
backtraces.  From script, one would pass the strings created by
backtrace().  The runtime printf could expand it to print_backtrace()
internally, or perhaps even, pass a code to user-space to do it there
later.

- FChE

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-04-04 19:31 -------
Subject: Re:  STP_STRING_SIZE set by stap is too small

On Tue, 2006-04-04 at 02:08 +0000, guij at cn dot ibm dot com wrote:

> I think _lket_trace() is more like:
> bin_write("%n%1b%4b%1b%ld%4b%4b%s",this_event_len(), cmd->device->sdev_state,
> [...], _stp_string_ptr(str))
> The total length of the trace record (including backtrace string) is to be
> calculated and filled by bin_write(), thus only #2 can work.

OK, I see the problem.  It seems the simplest thing is to just do what
you proposed and either increase STP_STRING_SIZE or make it definable on
the command line, or both The only remaining use of STP_STRING_SIZE is
for strings used while doing stack traces, so it seems like a good idea
to set it higher by default, because the current default is not useful.
Maybe 512 or 1024?  You can check in the change, or I will if you
prefer.

By the way, in the above example, what is the purpose of
this_event_len()?  It seems that you have defined "%n" to write the
length of the finished string to that location, but why does that need
an argument?

Martin





-- 


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

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

Re: [Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[Martin Hunt]

On Tue, 2006-04-04 at 02:08 +0000, guij at cn dot ibm dot com wrote:

> I think _lket_trace() is more like:
> bin_write("%n%1b%4b%1b%ld%4b%4b%s",this_event_len(), cmd->device->sdev_state,
> [...], _stp_string_ptr(str))
> The total length of the trace record (including backtrace string) is to be
> calculated and filled by bin_write(), thus only #2 can work.

OK, I see the problem.  It seems the simplest thing is to just do what
you proposed and either increase STP_STRING_SIZE or make it definable on
the command line, or both The only remaining use of STP_STRING_SIZE is
for strings used while doing stack traces, so it seems like a good idea
to set it higher by default, because the current default is not useful.
Maybe 512 or 1024?  You can check in the change, or I will if you
prefer.

By the way, in the above example, what is the purpose of
this_event_len()?  It seems that you have defined "%n" to write the
length of the finished string to that location, but why does that need
an argument?

Martin

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[guanglei at cn dot ibm dot com]

------- Additional Comments From guanglei at cn dot ibm dot com  2006-04-05 00:17 -------
(In reply to comment #11)
> OK, I see the problem.  It seems the simplest thing is to just do what
> you proposed and either increase STP_STRING_SIZE or make it definable on
> the command line, or both. The only remaining use of STP_STRING_SIZE is
> for strings used while doing stack traces, so it seems like a good idea
> to set it higher by default, because the current default is not useful.
> Maybe 512 or 1024?  
I think only making STP_STRING_SIZE definable is ok. But it may be better if we
can also increase the default value of STP_STRING_SIZE to 512, which should have
no side effect. 512 is my estimated and I think it should be enough.

> You can check in the change, or I will if you prefer.
Oh, I don't have the permissions to check into CVS. It's good if you can do this
for me.

> By the way, in the above example, what is the purpose of
> this_event_len()?  It seems that you have defined "%n" to write the
> length of the finished string to that location, but why does that need
> an argument?
this_event_len() will always 0 for LKET, the real length will be calculated at
the end of bin_vsnwrite. So this_event_len() is just showing that the current
argument represents the length of the data to be written.

-- 


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

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

[Bug runtime/2497] STP_STRING_SIZE set by stap is too small

[hunt at redhat dot com]

------- Additional Comments From hunt at redhat dot com  2006-04-12 19:21 -------
Fix committed.

      s.op->newline() << "#ifndef STP_STRING_SIZE";
      s.op->newline() << "#define STP_STRING_SIZE 1024";
      s.op->newline() << "#endif";


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|                            |FIXED


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

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