Re: [PATCH] Fix for the gdb.base/macscp.exp testcase failure with split dwarf.

[Keith Seitz <keiths@redhat.com>]

On 3/14/22 07:32, Achra, Nitika wrote:
> Thanks for the review and sorry for the delay in the reply.

No problem; it happens. [At least to me!]

I'm happy to see you return to the problem/patch.

> I have fixed the regressions mentioned by you. Also, I have removed
> the change you have posted for the RFC. I have attached the updated
> patch. So, this patch along with your fix will result in the
> following for gdb.base/macscp.exp testcase with -gsplit-dwarf:
>
> 
> Command Used:
> 
> make check RUNTESTFLAGS='CC_FOR_TARGET="clang -gdwarf-5 -gsplit-dwarf -fdebug-macro"'
> 
>                           TESTS="gdb.base/macscp.exp"

Just a cautionary note: When running C++ tests (gdb.cp), don't forget to
set CXX_FOR_TARGET, too.

> Compiler used: clang 12.0.0

I tested w/GCC and Clang 12 (Fedora 12.0.1-1.fc34)
GCC results show no regressions.

> *Before the patch:*
> 
> Number of expected passes            234
> Number of unexpected failures        65
> Number of known failures             40

I also see those results with origin/master.

> *After applying the attached patch and including your change also:*
> 
> Number of expected passes            319
> Number of unexpected failures        1
> Number of known failures             19

After applying both our patches, I have an equal number of PASSes,
but 4 unexpected failures. [which could mean just about anything]

> Clang is emitting both .debug_line and .debug_line.dwo sections and
> .debug_macro.dwo is pointing to the .debug_line.dwo section(file
> index in the .debug_macro.dwo is the index in the file_names array
> of .debug_line.dwo). However, GDB is reading only .debug_line section.
>
> In this patch, I have added the support for reading .debug_line.dwo 
> section and added m_dwo_include_dirs and m_dwo_file_names vectors in 
> struct line_header to store the directories and files respectively 
> and reading the file and directory names from these vectors whenever
> required.
I think there is still something missing. Regression testing the entire
testsuite (with both our patches), I see the following three issues
(w/clang; gcc is fine):

1. gdb.base/included.exp
(gdb) list integer
-18     int integer; [PASS]
+18     #include "included.h" [FAIL]

2. gdb.cp/m-static.exp
(gdb) info variable everywhere
-File ../../../src/gdb/testsuite/gdb.cp/m-static.h: [PASS]
+File ../../../src/gdb/testsuite/gdb.cp/m-static.cc: [FAIL]

3. gdb.threads/siginfo-threads.exp
(gdb) p $_siginfo.si_signo
-$4 = 3182372 [PASS]
+There is no member named _sifields. [FAIL]
(gdb) p $_siginfo._sifields._kill.si_pid
-$7 = 3182372 [PASS]
+There is no member named _sifields. [FAIL]

I haven't pursued these further to understand the problem.

See other discussion below.

Keith

> diff --git a/gdb/dwarf2/line-header.h b/gdb/dwarf2/line-header.h
> index 8fb44be56b2..48cf9a5b742 100644
> --- a/gdb/dwarf2/line-header.h
> +++ b/gdb/dwarf2/line-header.h
> @@ -44,9 +44,12 @@ struct file_entry
>        length (length_)
>    {}
>  
> -  /* Return the include directory at D_INDEX stored in LH.  Returns
> -     NULL if D_INDEX is out of bounds.  */
> -  const char *include_dir (const line_header *lh) const;
> +  /* Return the include directory at D_INDEX stored in m_include_dirs vector
> +     in LH or in m_dwo_include_dirs vector in LH if IS_DWO is true. m_dwo_
> +     include_dirs contains the include_directories entries of .debug_line.dwo
> +     section whereas m_include_dirs contains the include_directories array
> +     entries of .debug_line section. Returns NULL if D_INDEX is out of bounds.  */
> +  const char *include_dir (const line_header *lh, bool is_dwo) const;

Regarding passing a boolean everywhere (like above) and special-casing on this,
like:

> -  /* Return the include dir at INDEX (0-based in DWARF 5 and 1-based before).
> +  /* Return the include dir from m_include_dirs or from the m_dwo_include_dirs
> +     if IS_DWO is true at INDEX (0-based in DWARF 5 and 1-based before).
>       Returns NULL if INDEX is out of bounds.  */
> -  const char *include_dir_at (dir_index index) const
> +  const char *include_dir_at (dir_index index, bool is_dwo) const
>    {
>      int vec_index;
> -    if (version >= 5)
> +
> +    if (is_dwo && m_dwo_include_dirs.empty())
> +      is_dwo = false;
> +
> +    if ((is_dwo && dwo_version >= 5) || (!is_dwo && version >= 5))
>        vec_index = index;
>      else
>        vec_index = index - 1;
> -    if (vec_index < 0 || vec_index >= m_include_dirs.size ())
> +    int include_dirs_size = is_dwo ? m_dwo_include_dirs.size () :
> +			    m_include_dirs.size ();
> +    if (vec_index < 0 || vec_index >= include_dirs_size)
>        return NULL;
> -    return m_include_dirs[vec_index];
> +    return is_dwo ? m_dwo_include_dirs[vec_index] :
> +		    m_include_dirs[vec_index];
>    }

I've seen this implementation pattern elsewhere (mostly older code), and
I just can't help but ask... It seems to unnecessarily clutter the code compared
to creating a new struct/class, inheriting from the existing line_header. [In other
words, it seems C-ish to me.]

Was that approach considered?

Keith