Re: [0/4] RFC: add DWARF index support

[Doug Evans <dje@google.com>]

On Fri, Aug 6, 2010 at 10:39 AM, Tom Tromey <tromey@redhat.com> wrote:
> Doug> IWBN to add to the comment about exiting without creating an index not
> Doug> being an error, e.g. provide an example.
> Doug> Is it because the file could be stripped?  [If it is stripped, should
> Doug> the script fail or pass?  Dunno.]
>
> I don't want to do this, because the reasons may change.

I don't want to unnecessarily delay the patch (given, for example, its
size), but not generating an index file when asked to and not flagging
an error feels wrong at a gut level.
OTOH strip doesn't flag an error if the file is already stripped.
Maybe this is akin to that.
At any rate, the some examples of this behaviour may be things the
user could trip over, so we should document at least those reasons
somewhere, the script could just contain a reference to the docs.

E.g., I wonder what should happen if there isn't enough (or any) info
to generate .gdb-index.
Is that an error?
I just tried it, no index is saved and gdb doesn't flag an error.
I'm not sure I like that behaviour (but I wouldn't hold up the patch
because of it, I think we agree that the index generation should be
done elsewhere anyway), but can we agree that if we keep it it needs
to be documented (somewhere).  ["it" being cases like trying to
generate an index when there is no debug info present to begin with,
and anything else the user might easily trip over.]

[For completeness sake ...
OTOH, given that index generation should be done elsewhere anyway, I'm
more inclined to be rather strict with what is successful behaviour.
And if such things did flag an error, I think there's less need to
document them. :-)]

> Doug> IWBN to put "${file}.gdb-index" in its own variable so that there's
> Doug> just one instance.
>
> Ok.
>
> Doug> LGTM with the above nits.
>
> I don't know what LGTM means.

"Looks good to me."
[for reference sake, I did see where it ranked in google's search
results before using it. 1/2 :-)
I wasn't sure how common its use was (outside of google anyway).]

> How about this?
>
> Tom
>
> 2010-08-05  Tom Tromey  <tromey@redhat.com>
>
>        * gdb-add-index.sh: Add error checking.
>
> Index: gdb-add-index.sh
> ===================================================================
> RCS file: /cvs/src/src/gdb/gdb-add-index.sh,v
> retrieving revision 1.1
> diff -u -r1.1 gdb-add-index.sh
> --- gdb-add-index.sh    30 Jul 2010 20:46:34 -0000      1.1
> +++ gdb-add-index.sh    6 Aug 2010 17:39:20 -0000
> @@ -16,14 +16,29 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>
> +if test $# -ne 1; then
> +   echo "Usage: gdb-add-index FILE" 1>&2
> +   exit 1
> +fi
> +
>  file="$1"
>  dir="${file%/*}"
> +index="${file}.gdb-index"
>
> -gdb --batch-silent -ex "file $file" -ex "save gdb-index $dir"
> +gdb --batch-silent -ex "file $file" -ex "save gdb-index $dir" || {
> +   status=$?
> +   # Just in case.
> +   rm -f "$index"
> +   exit $status
> +}
>
> -if test -f "${file}.gdb-index"; then
> -   objcopy --add-section .gdb_index="${file}.gdb-index" --set-section-flags .gdb_index=readonly "$file" "$file"
> -   rm -f "${file}.gdb-index"
> +# In some situation gdb can exit without creating an index.  This is
> +# not an error.
> +status=0
> +if test -f "${index}"; then
> +   objcopy --add-section .gdb_index="${index}" --set-section-flags .gdb_index=readonly "$file" "$file"
> +   status=$?
> +   rm -f "${index}"
>  fi
>
> -exit 0
> +exit $status
>

Ok.

I hate to add another wrinkle, and thanks for bearing with me.
I just tried passing a file that doesn't exist.  gdb writes something
to stderr and then exits with a zero exit code.
That needs to be flagged as an error (presumably with test -r or some
such before invoking gdb) , but it could be deferred to another patch.