[PATCH] add debug information to v2 builds

[Stefan Reinauer]

see attached patch.

[Carl-Daniel Hailfinger]

On 27.11.2008 14:40, Stefan Reinauer wrote:
> This patch from Ralf Grosse Boerger makes debugging more comfortable. 
> With this patch it's possible to
> determine the according source code line for each asm statement
> (objdump -dS)
> determine the source code file for each asm statement 
> (objdump -ddl)
>
> This isn't exactly trivial because cache_as_ram_auto.c gets compiled to
> assembly and converted by a perl script afterwards.
>
> This patch solves the problem
> by extending cache_as_ram_auto.inc with debug information and line numbers
> by correcting the perl calls (".text" --> ".text")
> by creating a disassembly with source code and line numbers. (ctr0.disasm and
> coreboot.disasm)
>
> There's one minor downside to the patch: A complete abuild run takes up around
> 1.6G instead of about 700MB now. But I'm sure this is quite reasonable for the
> benefits.
>
> Signed-off-by: Stefan Reinauer

It would be great if you could split the regular expression bugfix from
the rest of the patch. That makes reviewing a lot easier and the
individual patches are smaller.

The perl regexp change is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

[Patrick Georgi]

Am Donnerstag, den 27.11.2008, 17:18 +0100 schrieb Carl-Daniel
Hailfinger:
> It would be great if you could split the regular expression bugfix
> from
> the rest of the patch. That makes reviewing a lot easier and the
> individual patches are smaller.
>
> The perl regexp change is
> Acked-by: Carl-Daniel Hailfinger

Besides this, there are only two more parts:
1. add more flags to compilation of all mainboards (debug symbols, line
numbers, verbose assembly)
2. create .disasm files of crto and coreboot, containing the disassembly
(in src/config/Config.lb)

I did a full abuild run with the debug symbols on, using a payload that
leaves barely enough space for coreboot without these changes in most
ROMs (4kb or so more, and many of the boards failed).
All boards built just the same with debug symbols, so increase in size
(if any) should be acceptable.

The only drawback was that the tree was much larger, as the non-stripped
coreboot binary was 2-3 times the size, the disassembly of all coreboot
images takes some space, ...

I first thought about splitting the regex off, too - but seriously, the
other stuff is just as repetitive (same file, even), and imho
functionally trivial (but potentially a large impact, thus the tests)

Regards,
Patrick Georgi

[Uwe Hermann]

On Thu, Nov 27, 2008 at 02:40:43PM +0100, Stefan Reinauer wrote:
> This patch from Ralf Grosse Boerger makes debugging more comfortable. 
> With this patch it's possible to
> determine the according source code line for each asm statement
> (objdump -dS)
> determine the source code file for each asm statement 
> (objdump -ddl)
>
> This isn't exactly trivial because cache_as_ram_auto.c gets compiled to
> assembly and converted by a perl script afterwards.
>
> This patch solves the problem
> by extending cache_as_ram_auto.inc with debug information and line numbers
> by correcting the perl calls (".text" --> ".text")
> by creating a disassembly with source code and line numbers. (ctr0.disasm and
> coreboot.disasm)
>
> There's one minor downside to the patch: A complete abuild run takes up around
> 1.6G instead of about 700MB now. But I'm sure this is quite reasonable for the
> benefits.

Can we make this optional, please? 700MB is already quite a lot, let alone 1.6GB.

The changes are fine with me, but there should be some option somewhere
to not generate the extra 900MB of debug output if you don't want it.

Thanks, Uwe.

[Stefan Reinauer]

Uwe Hermann wrote:
> On Thu, Nov 27, 2008 at 02:40:43PM +0100, Stefan Reinauer wrote:
> > This patch from Ralf Grosse Boerger makes debugging more comfortable. 
> > With this patch it's possible to
> > determine the according source code line for each asm statement
> > (objdump -dS)
> > determine the source code file for each asm statement 
> > (objdump -ddl)
> >
> > This isn't exactly trivial because cache_as_ram_auto.c gets compiled to
> > assembly and converted by a perl script afterwards.
> >
> > This patch solves the problem
> > by extending cache_as_ram_auto.inc with debug information and line numbers
> > by correcting the perl calls (".text" --> ".text")
> > by creating a disassembly with source code and line numbers. (ctr0.disasm and
> > coreboot.disasm)
> >
> > There's one minor downside to the patch: A complete abuild run takes up around
> > 1.6G instead of about 700MB now. But I'm sure this is quite reasonable for the
> > benefits.
>
> Can we make this optional, please? 700MB is already quite a lot, let alone 1.6GB.
>
> The changes are fine with me, but there should be some option somewhere
> to not generate the extra 900MB of debug output if you don't want it.

The v2 build system is not exactly friendly in allowing stuff like this.
What do you suggest?

[Carl-Daniel Hailfinger]

On 27.11.2008 20:24, Uwe Hermann wrote:
> On Thu, Nov 27, 2008 at 02:40:43PM +0100, Stefan Reinauer wrote:
> > This patch from Ralf Grosse Boerger makes debugging more comfortable. 
> > With this patch it's possible to
> > determine the according source code line for each asm statement
> > (objdump -dS)
> > determine the source code file for each asm statement 
> > (objdump -ddl)
> >
> > This isn't exactly trivial because cache_as_ram_auto.c gets compiled to
> > assembly and converted by a perl script afterwards.
> >
> > This patch solves the problem
> > by extending cache_as_ram_auto.inc with debug information and line numbers
> > by correcting the perl calls (".text" --> ".text")
> > by creating a disassembly with source code and line numbers. (ctr0.disasm and
> > coreboot.disasm)
> >
> > There's one minor downside to the patch: A complete abuild run takes up around
> > 1.6G instead of about 700MB now. But I'm sure this is quite reasonable for the
> > benefits.
>
> Can we make this optional, please? 700MB is already quite a lot, let alone 1.6GB.
>
> The changes are fine with me, but there should be some option somewhere
> to not generate the extra 900MB of debug output if you don't want it.

That's also one of the reasons why I didn't ack the complete patch. I do
coreboot development on my work machine which happens to be a laptop
with not enough hard disk space. I would have to delete some stuff to
run the new abuild.

AFAICS abuild images are not fully bootable anyway because they have no
payload. I may be wrong, though. However, if I'm right, the debug
symbols in abuild don't help anyone. Debug symbols in "real" builds
would make sense.

Regards,
Carl-Daniel

[Patrick Georgi]

Am Donnerstag, den 27.11.2008, 20:38 +0100 schrieb Carl-Daniel
Hailfinger:
> That's also one of the reasons why I didn't ack the complete patch. I do
> coreboot development on my work machine which happens to be a laptop
> with not enough hard disk space. I would have to delete some stuff to
> run the new abuild.

Of the 1.6GB, nearly 400mb are the .disasm files, so it splits up as:
700mb baseline
400mb disasm
500mb debug symbols in object files

It would be trivial to comment out the disasm commands, so those files
aren't created by default. (the new lines in src/config/Config.lb)
It might also be reasonably simple to enable them with some option.

As for the debug symbols, I don't know enough about the build system to
say how complex it is to enable/disable those options "on demand". Given
that they're necessary in each mainboard directory, that looks harder
for me.

Regards,
Patrick Georgi

[Carl-Daniel Hailfinger]

On 27.11.2008 20:56, Patrick Georgi wrote:
> Am Donnerstag, den 27.11.2008, 20:38 +0100 schrieb Carl-Daniel
> Hailfinger:
> > That's also one of the reasons why I didn't ack the complete patch. I do
> > coreboot development on my work machine which happens to be a laptop
> > with not enough hard disk space. I would have to delete some stuff to
> > run the new abuild.
>
> Of the 1.6GB, nearly 400mb are the .disasm files, so it splits up as:
> 700mb baseline
> 400mb disasm
> 500mb debug symbols in object files
>
> It would be trivial to comment out the disasm commands, so those files
> aren't created by default. (the new lines in src/config/Config.lb)
> It might also be reasonably simple to enable them with some option.

Cool.
> As for the debug symbols, I don't know enough about the build system to
> say how complex it is to enable/disable those options "on demand". Given
> that they're necessary in each mainboard directory, that looks harder
> for me.

In theory, each src/mainboard/FOO/Config.lb will be turned into a
makefile, so a makefile variable for the added parameters should work.

Regards,
Carl-Daniel

[Peter Stuge]

Patrick Georgi wrote:
> As for the debug symbols,

Please commit while this is being worked out.

Acked-by: Peter Stuge

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > Can we make this optional, please? 700MB is already quite a lot, let alone 1.6GB.
> >
> > The changes are fine with me, but there should be some option somewhere
> > to not generate the extra 900MB of debug output if you don't want it.
>
> That's also one of the reasons why I didn't ack the complete patch. I do
> coreboot development on my work machine which happens to be a laptop
> with not enough hard disk space. I would have to delete some stuff to
> run the new abuild.
>
> AFAICS abuild images are not fully bootable anyway because they have no
> payload. I may be wrong, though. However, if I'm right, the debug
> symbols in abuild don't help anyone. Debug symbols in "real" builds
> would make sense.

Even with the most expensive laptop disks, the price per gigabyte is at
20-60 euro cents.

Are we really having this discussion in 2008?

[Uwe Hermann]

On Fri, Nov 28, 2008 at 01:09:10PM +0100, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > > Can we make this optional, please? 700MB is already quite a lot, let alone 1.6GB.
> > >
> > > The changes are fine with me, but there should be some option somewhere
> > > to not generate the extra 900MB of debug output if you don't want it.
> >
> > That's also one of the reasons why I didn't ack the complete patch. I do
> > coreboot development on my work machine which happens to be a laptop
> > with not enough hard disk space. I would have to delete some stuff to
> > run the new abuild.
> >
> > AFAICS abuild images are not fully bootable anyway because they have no
> > payload. I may be wrong, though. However, if I'm right, the debug
> > symbols in abuild don't help anyone. Debug symbols in "real" builds
> > would make sense.
>
> Even with the most expensive laptop disks, the price per gigabyte is at
> 20-60 euro cents.
>
> Are we really having this discussion in 2008?

Yes, I'm with Carl-Daniel on this issue. I'm also working on a laptop
almost always (but this also applies to desktop PCs as well), and even
though I upgraded my drive to be double the size recently, that space
was consumed away in just 1-2 days. There's never be enough disk space,
no matter how big the drive.

As an example, I currently have less than 1 GB of free disk space. I'm
already using up ca. 1.2 GB just for v2 and v3 checkouts, let alone
various other git/svn/cvs checkouts of other projects I'm involved with
or interested in, and lots of other stuff of course. So yes, disk space
issues cannot be ignored, not even in 2008, IMHO.

However, another way to solve this may be to delete each board again
after it has been built with abuild. As a developer using abuild you
usually only want to know if all boards will still build fine after
your patch, you don't really need to keep all the build output around.
Shall we add such an abuild option maybe?

Uwe.