Re: [PATCH][gnulib] Add the Sframe package

[Weimin Pan]

Hi Bruno,

Thanks for your comments.

On 11/15/2022 4:52 PM, Bruno Haible wrote:
> Hello Weimin,
>
> > The main use-case for this format are
> > "online" debugging tools like stack tracers
> I'll appreciate anything that can help producing a universally working
> backtrace for C, since experience (e.g. from Lisp, Java, Python) has
> shown that such a backtrace function can tremendously improve the
> developer productivity and the usefulness of bug reports from anonymous
> users.
>
> My experiments on this topic, two years ago, were not satisfactory:
>    * The backtrace function from glibc
>        - produces no file names, no line numbers,
>        - produces function names only when the binary was linked with
>          '-rdynamic',
>        - is not portable.
>    * The libbacktrace library produced hardly useful information as well, even
>      in simple cases. See [1].
>
> Can you describe how the SFrame format fits in, w.r.t. to
>    - backtrace from glibc (based on libunwind AFAIK),
>    - libbacktrace ?

We came to design the SFrame format (The S stands for `simple') due to some
concrete requirements of a very big program that ships its own "online"
stack tracer and unwinder to handle error conditions:

1) They wanted something simple to decode and simple to compute.  This
   is in contrast with DWARF/eh_frame that, generally, requires
   executing DWARF expressions and therefore the maintenance of a small
   stack machine.

2) They wanted the unwind info to be fast to decode and to compute.

3) They wanted the unwind info to be as compact as possible, of course
   maintaining a good trade-off vs. simplicity.

4) They wanted something that wouldn't be stripped out of the
   executables or shared objects, even maybe as a mistake.

5) They wanted something to the point: just unwind information, i.e. the
   ability of jumping back the stack frames.  This doesn't include type
   information, the contents of every callee-saved register, etc.

Now, both libbacktrace and libunwind use eh_frame or DWARF as an
underlying format from which to extract the unwind info.  We could (and
have considered) adding support for SFrame as well to these libraries,
but that is orthogonal to adding the modules (definition of the stored
format data structures, and convenience encoders/decoders) to gnulib:
the applications using the modules will most likely implement its own
stack tracer and debug infrastructure.

> > This patch adds two new modules to gnulib, related to the Simple Frame
> > format (SFrame) recently introduced in binutils [1].
> I'm pleased to see that it is extensively commented, includes sanity checks,
> and is already in GNU coding style.
>
> Since this is a significant contribution (regardless whether it finally
> goes into Gnulib [2] or into Binutils [3]), we will need a copyright assignment
> to the FSF for this code. Are you already aware how this works, in your context
> as an Oracle employee? Maybe José Marchesi can share his experience on this
> topic with you?

Yes, I believe this issue has been addressed and resolved.

> Once the big picture is clear and the copyright issue is handled, we'll
> go into the detailed code review. For now, what I can see are only nits,
> due to Gnulib conventions:
>    - The header file module should better be named 'sframe-h' instead of
>      'sframe-header'.

OK, done.

>    - The *.c and *.h files should be untabified (run 'expand -t 8' on each).

OK, will do.

>    - tests/sframe-api.h should not be included since it's a copy of
>      lib/sframe-api.h, right?

Yes, it is and will be deleted.

>    - tests/DATA* should better be moved to a subdirectory:
>      tests/sframe-data/DATA*, to make it clear to which part of Gnulib it
>      belongs.

OK.

> Bruno
>
> [1] https://github.com/ianlancetaylor/libbacktrace/issues
> [2] https://www.gnu.org/software/gnulib/manual/html_node/Copyright.html
> [3] 
> https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment