Re: [PATCH v2 2/2 gdb] Fix --enable-libctf, --disable-static, and tests sans CTF-capable toolchain

[Nick Alcock <nick.alcock@oracle.com>]

On 15 Oct 2019, Tom Tromey said:

>>>>>> "Nick" == Nick Alcock <nick.alcock@oracle.com> writes:
>
> Thanks for doing this.  This looks basically ok, just a few nits below.

Thanks for the review!

> The patch should have some intro text.  gdb style is to use the same
> text for the description in the email and for the commit.

I know: almost uniquely for me, I couldn't think of anything. :) I was
hoping you'd recommend something that wasn't literally repeating the
subject line...

(I also need to change things to cite the ChangeLog names in the commit
changelog text copy, rather than just the dir as binutils does.)

> Nick> +if test x${enable_static} = xno; then
> Nick> +  LIBCTF="-Wl,--rpath,../libctf/.libs ../libctf/.libs/libctf.so"
>
> Indentation of 2 here...
> 
> Nick> +if test "${enable_libctf}" = yes; then
> Nick> +    AC_DEFINE(ENABLE_LIBCTF, 1, [Handle .ctf type-info sections])
>
> ... but 4 here.  I think they could be the same, whatever is most common
> in the file, or most common nearby anyhow.

Oops! You can tell I was editing several sorts of thing at once with
different indentatn levels: fixed.

> Nick> +#else
> Nick> +
> Nick> +void
> Nick> +_initialize_ctfread (void)
>
> gdb just uses "()", not "(void)" now.

Oh good! C++ to the rescue etc. Fixed. (And the other instance I copied
it from, further up in the file.)

> Nick> diff --git a/gdb/testsuite/configure.ac b/gdb/testsuite/configure.ac
> ... 
> Nick> +AC_ARG_ENABLE(libctf,
> Nick> +[AS_HELP_STRING([--enable-libctf], [Handle .ctf type-info sections])], [
>
> This seems to be identical to what's in gdb.
> 
> I don't really mind, but if you're duplicating this across several
> subdirectories, it may be better to stick a new macro into a new .m4 in
> config/, then use that everywhere.

I was thinking exactly the same thing last night. It's now duplicated in
four places, clearly way above any reasonable refactoring bound... I'll
adjust both patches in the series accordingly.

> Nick> +if {[skip_ctf_tests]} {
> Nick> +    return
> Nick> +}
>
> I think these spots should call "unsupported" to explain what happened.

I didn't notice that existed. Thank you, will do!

Something like

unsupported "No compiler CTF support, or CTF disabled in GDB"

I suppose.

> Nick> +proc skip_ctf_tests {} {
>
> I suspect you probably want to use gdb_caching_proc here.  This can be
> friendlier when running a lot of parallel tests, because it caches the
> result, and this proc is doing a compilation.

I was hoping something like this existed too. (You can tell I'm a newbie
to the GDB testsuite.)

Hm let's see if it has any special requirements... doesn't look like it,
other than that the output is unvarying.

New series coming soon. :) (Changes on the binutils side, as well, for
the --enable-libctf refactoring.)