Re: [RFC][PATCH] fix gdb segv when objfile can't be opened

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-10-30 18:13, Mike Gulick wrote:
> On 10/27/2017 09:17 PM, Simon Marchi wrote:
>> On 2017-10-23 07:19 PM, Mike Gulick wrote:
>> 
>> ...
>> 
>> Thanks a lot.  Could you also write the corresponding ChangeLog 
>> entries?  See
>> here for details: http://sourceware.org/gdb/wiki/ContributionChecklist
>> Note that we usually put that as part of the commit log, and only move 
>> it
>> to the actual ChangeLog files before pushing to git.  The changes to 
>> the
>> source files will go in gdb/ChangeLog and the changes to the testsuite 
>> in
>> gdb/testsuite/ChangeLog.
>> 
>> You can also put the PR number as part of the ChangeLog (see wiki and
>> previous CL entries for examples).
>> 
> Done.
> 
>> I don't think you have a copyright assignment for GDB yet?  Since this 
>> is
>> more than a few lines, you will need one if you want to contribute 
>> (I'll
>> contact you off-list for that).
>> 
> I'm working on this.  Hopefully it will be sooner rather than later.
> 
>> I noted a a few comments below (mostly minor/formatting stuff, overall 
>> I
>> think this is a good quality submission).
>> ...
>>> 
>>> diff --git a/gdb/gdb_bfd.c b/gdb/gdb_bfd.c
>>> index cc02740..b33de07 100644
>>> --- a/gdb/gdb_bfd.c
>>> +++ b/gdb/gdb_bfd.c
>>> @@ -702,9 +702,14 @@ gdb_bfd_map_section (asection *sectp, 
>>> bfd_size_type *size)
>>> 
>>>    data = NULL;
>>>    if (!bfd_get_full_section_contents (abfd, sectp, &data))
>>> -    error (_("Can't read data for section '%s' in file '%s'"),
>>> -	   bfd_get_section_name (abfd, sectp),
>>> -	   bfd_get_filename (abfd));
>>> +    {
>>> +      warning (_("Can't read data for section '%s' in file '%s'"),
>>> +	       bfd_get_section_name (abfd, sectp),
>>> +	       bfd_get_filename (abfd));
>>> +      /* Section is invalid -- set size to 0 and return NULL */
>> 
>> Finish the comment with a dot and two spaces.
>> 
> I rewrote the comment to be a full sentence

Ok.

>>> ...
>>> +extern int bar (int);
>>> +
>>> +int foo (int x) {
>>> +  return bar (x);
>>> +}
>> 
>> Unless the purpose of the test is about formatting, we try to follow 
>> GNU
>> style (same as for source code) in tests.  For example, here it would 
>> be:
>> 
>> int
>> foo (int x)
>> {
>>   return bar (x);
>> }
>> 
> I reformatted all of the helper files for the test using 'indent'.

Great, thanks.

>>> ...
>>> +int bar (int y) {
>>> +  return y + 1; /* break here */
>>> +}
>> 
>> Same here.
>> 
>>> ...
>>> +#ifndef VANISH_LIB
>>> +#define VANISH_LIB "./solib-vanish-lib1.so"
>>> +#endif
>> 
>> I think we can remove this, it could only obfuscate a problem
>> if we remove the -DVANISH_LIB in the .exp file.
>> 
> Done.
> 
>>> +
>>> +int main()
>> 
>> Put "int" on its own line.
>> 
>>> +{
>>> +  void *handle;
>>> +  int (*foo)(int);
>>> +  char *error;
>>> +
>>> +  /* Open library */
>>> +  handle = dlopen(VANISH_LIB, RTLD_NOW);
>>> +  if (!handle) {
>>> +    fprintf(stderr, "%s\n", dlerror());
>> 
>> Please make this file follow GNU style as well (space before 
>> parenthesis,
>> curly braces position, etc).
>> 
>>> +    exit(EXIT_FAILURE);
>>> +  }
>>> +
>>> +  /* Clear any existing error */
>>> +  dlerror();
>>> +
>>> +  /* Remove library */
>>> +  if (remove(VANISH_LIB) == -1) {
>> 
>> Can we rename the lib instead of deleting it?  It's always preferable 
>> to keep
>> test artifacts in case we need to inspect them.
>> 
> I updated the test to rename the library instead of removing it.  It
> also now renames it
> back before exiting.  This *should* make the test idempotent, unless
> it fails before it
> is able to rename the .so back to its original name.

The test (the .exp) rebuilds the shared library every time anyway, so 
even if you didn't move it back, it would work fine.

>>> ...
>>> +
>>> +# Library 1
>>> +set lib1name "solib-vanish-lib1"
>>> +set srcfile_lib1 ${srcdir}/${subdir}/${lib1name}.c
>>> +set binfile_lib1 [standard_output_file ${lib1name}.so]
>>> +# I can't make this work for some reason:
>>> +#set lib1_flags [list debug shlib=${binfile_lib2}]
>>> +set lib1_flags [list debug ldflags=-l:${lib2name}.so 
>>> ldflags=-Wl,-rpath=[standard_output_file ""] 
>>> libdir=[standard_output_file ""]]
>> 
>> With
>> 
>>   https://sourceware.org/ml/gdb-patches/2017-10/msg00856.html
>> 
>> the commented out line should work.
>> 
> This did indeed work.  Thank you!
> 
>>> ...
>>> +
>>> +gdb_test "continue" \
>>> +    "Breakpoint .*at .*/${lib2name}.c:${lib2_lineno}.*break here.*" 
>>> \
>>> +    "breakpoint prints location"
>> 
>> I think you could use the "gdb_continue_to_breakpoint" proc.
>> 
> This works as well, so the new patch uses this instead.
> 
> Here's an updated patch which hopefully addresses all of these items.
> 
> ...

Thanks for the update, this LGTM.  Please ping when your hear back about 
your copyright assignment.

Simon