Re: [PATCH] Disassemble over end of line table sequence.

[Joel Brobecker <brobecker at adacore dot com>]

Andrew,

First, I apologize for the delay in reviewing your patch.  Looks like
we've all been very busy the last few weeks.

> 2010-12-10  Andrew Burgess  <aburgess@broadcom.com>
> 
>           * (compare_lines): Sort by pc for entries where the line
>             number is 0, these are the end of sequence markers.

I think that the patch is correct in principle, there are are a few nits
that need to be fixed before it can go in.  See below.

Note that ever line in the CL entry needs to be aligned - I can't remember
if it was you already told about this. I may have, this patch is from
4 weeks ago...

> gdb/testsuite/
> 
> 2010-12-10  Andrew Burgess  <aburgess@broadcom.com>
> 
>           * gdb.disasm/disasm-end-cu-1.c, gdb.disasm/disasm-end-cu-2.c,
>             gdb.disasm/disasm-end-cu.exp: New test for disassembling
>             over the boundary between two compilation units.

I think you got mislead a bit by the directory name when choosing
the directory where to put your testcase. This directory contains
testcase that use files with assembly code, and thus limited to
selected architectures.  Yours uses plain C files, which can be compiled
on all hosts.  So, let's move your testcase to gdb.base.

> +  /* Work with end of sequence markers
> +     where the line number is set to 0 */

The lines are too short - please join them into 1 (actually, the guideline
is 70 characters, and that means it doesn't fit, but let's make the first
line a little longer, which is a more natural length). Also, the GNU Coding
Standards (GCS) require that we put a period at the end of every sentence,
and periods are followed by 2 spaces. Thus:

  /* Work with end of sequence markers where the line number is set
     to 0. */

> +  if ( mle1->line == 0 || mle2->line == 0 )

Formatting, no space after '(' and before ')':

  if (mle1->line == 0 || mle2->line == 0)

> +      if ( val == 0 )

Same here.

> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu-1.c
> @@ -0,0 +1,14 @@
> +#include <stdio.h>

This file needs a copyright header.

> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu-2.c
> @@ -0,0 +1,13 @@
> +#include <stdio.h>

Same here.

> +++ b/gdb/testsuite/gdb.disasm/disasm-end-cu.exp
> @@ -0,0 +1,70 @@
> +# Copyright 2010 Free Software Foundation, Inc.

Can you add 2011? (And a `(C)' - this is not strictly mandated by the FSF,
but the script I'm planning on using to perform yearly updates requires it).

> +# This test can only be run on targets which support DWARF-2 and use gas.
> +# For now pick a sampling of likely targets.
> +if {![istarget *-*-linux*]
> +    && ![istarget *-*-gnu*]
> +    && ![istarget *-*-elf*]
> +    && ![istarget *-*-openbsd*]
> +    && ![istarget arm-*-eabi*]
> +    && ![istarget powerpc-*-eabi*]} {
> +    return 0

This is not necessary in your case.

> +send_gdb "disassemble /m ${main_addr},${dummy_3_addr}\n"
> +gdb_expect {
> +    -re "Dump of assembler code from ${main_addr} to

We should not be using gdb_send/gdb_expect.  Can you use gdb_test_multiple
instead?

> +gdb_stop_suppressing_tests;

You can remove this line.

-- 
Joel