Re: backtrace support

[Marco Gerards]

Vincent Guffens <address@hidden> writes:

Hi Vincent,

> Here is a new version of the backtrace support. I have written the
> ChangeLog and modified the code according to your advices.

Thanks a lot!  Here are still some comments and questions about the
patch.

> To compile with the backtrace support use ./configure --with-debug and
> load grub2 with the kern_debug module. The kern module will be
> unloaded automatically after having registered all the kernel symbols
> in a linked list that also contains the module symbols. Any files that
> includes <grub/backtrace.h> can now call grub_backtrace().

I'm not too familiar with autoconf, but I had the idea that
--enable-debug would be more logical and that --with is to tell
configure where to find libraries.  According to the autoconf manual
it is to add external software like libraries.

I have the idea that another array for the symbols is not required.  I
think we can use one common table.  The symbol name and symbol address
are already in there.  The hard thing is would getting the size,
perhaps we could leave that open and fill it in with a function, or
can we use a ld/gcc feature to do that?  This will make the backtrace
code simpler and it saves memory.  Do you think that is somehow
possible?

> The functions that handle the linked list with the debug symbols are
> generic and are put in kern/backtrace.c. The function grub_backtrace()
> however is architecture dependant and is put in
> kern/i386/pc/backtrace.c.
>
> This is a problem because I would say that it breaks compilation for
> other architectures. I think that the other architectures should have
> a function grub_backtrace() that does nothing, or even better that
> really does print a backtrace(). However, I don't know how to do it
> and I can't test it anyway.

I can fix this for the PPC when it is applied, that's not a problem.

> I have also slightly modified the way genmk.rb generates the rule to
> create the modules. I hope this slight change does not do anything
> silly that I missed.

Hopefully Okuji can have a look at these changes.  I don't know ruby
and this code. :(

> I hope this backtrace will be usefull, but not too much !

It will be. :-)

> +     * conf/i386-pc.rmk : Added kern/backtrace.c, kern/i386/pc/backtrace.c 
> +     in the kernel dependancy list and backtrace.h in the header list. Added 
> a
> +     new module kern_debug.mod.

Please be specific about to which variables they are added.  You can
see how we did that in the past in ChangeLog.
 +
> +     * grub2/configure.ac : Added a configure switch : --with-debug. 
> +     Turn the gcc optimization flag to -O0 and define a variable 
> GRUB_DEBUG=1 
> +     in Makefile.in if this switch is used. 

Same here.

> +     * gendebugkern.sh : New file : shell script which generates the debug
> +     symbols for the kernel.

Just saying "New file." would be enough.  After that you can say
"Likewise." for every other new files that follows this line.  Please
make sure the ":" follows the filename directly without a space, and
start the sentence with a capital letter.

Same for the rest of the changelog and please tell which function is
changed.  Have a look at the ChangeLog for an example.



As for the sourcecode, I have seen a lot of incorrect indention, etc.
It does not yet comply with the GCS (which is the GNU Coding Style and
also describes the changelog format:
http://www.gnu.org/prep/standards/).  Can you please have a look at
this, a consistent coding style is very important for a big problem
like GRUB.  If you want me to help you or point you to some problems
in the code which you can't find after reading this, just ask me so I
can help you.

Thanks,
Marco