Re: [PATCH] bug fix for gdb 16039

["Dave.Tian" <xhengdf@gmail.com>]

Testcase below:

----Makefile----
so:
      g++ -shared -static-libstdc++ -static-libgcc -fPIC -rdynamic
dy.cxx -m32 -o libdy.so
all: so
      g++ -m32 -ldl main.cxx -g -o main

----dy.cxx----
#include <string>
using namespace std;

extern "C" const char * getVerson()
{
    std::string verson = "1.0";
    return verson.c_str();
}

----main.cxx----
#include <dlfcn.h>
#include <stdio.h>
#include <string>
#include <iostream>
using namespace std;

int main()
{
    void *handle = dlopen("libdy.so", RTLD_LAZY);
    if (!handle) {
        fprintf (stderr, "%s\n", dlerror());
        return 1;
    }

    typedef const char*(*FUNCTION)();
    FUNCTION f = (FUNCTION)dlsym(handle, "getVerson");
    std::cout << f();

    dlclose(handle);

    // 'next' cmd stop work here
    int a, b, c;
    a = 1;
    b = 2;
    c = 3;
    std::cout << a << b << c;
}

thank you,
-YT

2013/10/19 Sergio Durigan Junior <sergiodj@redhat.com>:
> On Wednesday, October 16 2013, Dave Tian wrote:
>
>> 1 Description: Gdb bug 16039 created by me
>>
>>   Title: Gdb next" command stop working when shared library unloaded.
>>   Root Cause: There is "libc++" static linked into the shared library,
>> and since gdb insert internal breakpoints on std::terminate/longjump/...
>> so after dlclose, the memory address is
>> invalid,remove_breakpoints/insert_breakpoints
>> failed with EIO error.
>>
>>   Fix: Disable the internal breakpoints when dlclose hit.
>>
>> 2 ChangeLog:
>>
>> 2013-10-16  Tian Ye  <xhengdf@gmail.com>
>>
>>         PR gdb/16039
>>         * breakpoint.c (is_removable_in_unloaded_shlib): New function.
>>         (set_longjmp_breakpoint): Check if breakpoint's location
>>         address is not in an unloaded shared library
>>         (disable_breakpoints_in_unloaded_shlib): Disable the
>>         internal breakpoints in the hook function of shared
>>         library unload.
>
> Thanks for the patch.
>
> Just FYI: the formatting of this ChangeLog entry is wrong because it
> doesn't use TABs.  However, the entry you posted on the previous version
> of this patch was right, so I think this was just a copy & paste done
> wrong :-).  Anyway, just FYI as I said.
>
> I would also like to point that you still haven't contacted me about the
> copyright assignment.  You need to do that paperwork before your patch
> can be committed (don't worry, it's just this time).  Just send me an
> email offlist and I can send you the instructions.
>
>> 3 Patch Diffs:
>>
>> --- breakpoint.c.bak  2013-10-12 01:15:09.044081000 -0700
>> +++ breakpoint.c      2013-10-15 23:10:50.241167000 -0700
>> @@ -1118,6 +1118,23 @@ is_tracepoint (const struct breakpoint *
>>    return is_tracepoint_type (b->type);
>>  }
>>
>> +/* Return 1 if B is an internal memory breakpoint that
>> + * can be removed when shlib is unloaded, or 0 otherwise.  */
>> +
>> +static int
>> +is_removable_membreak_in_unloaded_shlib (const struct breakpoint *b)
>> +{
>> +  return (b->type == bp_longjmp
>> +       || b->type == bp_longjmp_resume
>> +       || b->type == bp_longjmp_call_dummy
>> +       || b->type == bp_exception
>> +       || b->type == bp_exception_resume
>> +       || b->type == bp_std_terminate
>> +       || b->type == bp_longjmp_master
>> +       || b->type == bp_std_terminate_master
>> +       || b->type == bp_exception_master);
>> +}
>
> Thanks for extending this function, but I still think it's incomplete.
> My rationale here is: I liked your previous idea of creating a function
> to identify whether a breakpoint is internal.  I think it is more
> complete this way.  However, such a function (IMO) should cover all the
> known possibilites of internal breakpoints.  And your list is not
> exhaustive, for sure.  So, IMO, you could take a closer at the possible
> internal breakpoints and properly extend this list.  Also, when you do
> that, you could rename your function to "is_internal_breakpoint".
>
> Well, this is my opinion.  I don't know what others think.  But I
> certainly don't like this new function the way it is.
>
> The rest of the patch looks good to me, but you should also provide a
> testcase for it as Tom noted.
>
> Thanks a lot for doing that,
>
> --
> Sergio