Fix a crash in compile_to_object function

  On non-Linux platforms, gdb crashes when compile command is issued
because of the null pointer in struct osabi_names gdb_osab. The attached
patch adds a check to avoid this crash and adds osabi name for Solaris.
However, there is probably more work required to enable compile feature
on Solaris (e.g., solaris_infcall_munmap) and other platforms.

Thanks,
Libor
  

On 9/6/19 6:37 AM, libor.bukata@oracle.com wrote:
> On non-Linux platforms, gdb crashes when compile command is issued
> because of the null pointer in struct osabi_names gdb_osab. The attached
> patch adds a check to avoid this crash and adds osabi name for Solaris.
> However, there is probably more work required to enable compile feature
> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
> 

Thank you for the patch! The compile feature, as you have discovered, needs
much more testing on non-linux configurations.

> --- gdb-8.3/gdb/compile/compile.c	2019-08-19 13:07:57.669785758 +0000
> +++ gdb-8.3/gdb/compile/compile.c	2019-08-19 13:07:33.865626973 +0000
> @@ -697,7 +697,7 @@ compile_to_object (struct command_line *
>        const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>  
>        /* Allow triplets with or without vendor set.  */
> -      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
>        compiler->set_triplet_regexp (triplet_rx.c_str ());
>      }

I'm not sure about this. Should os_rx be NULL (which you've shown is possible
for a number of configurations), this would leave triplet_rx as $ARCH(-[^-]*)?-",
e.g., "x86_64(-[^-]*)?-". I would call that a malformed regexp for this purpose.

While the plugin may handle that gracefully, this just doesn't seem very
user-friendly to me, but I am not a maintainer, so you should definitely
wait for a real maintainer to chime in.

Otherwise, the only comments I have relate to

> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");

We prefer explicit comparisons with NULL/nullptr.

Omitting the true-case expression is unusual. Not invalid, but certainly
unusual (in gdb). I find no uses of this idiom in our sources. I would
prefer to see the more explicit

   os_rx != nullptr ? os_rx : ""

but I'll let official maintainers chime in on this usage. I'm just as curious
to see how others feel about it.

Keith
  

Hi Keith,

I have no objections to use 'os_rx != NULL ? os_rx : ""' instead; it is 
more portable. Note that NULL should be used instead of nullptr as 
nullptr is available since C++11.

Thanks,
Libor

feel free to use 'os_rx != nullptr ? os_rx : ""' instead

On 9/6/19 6:42 PM, Keith Seitz wrote:
> On 9/6/19 6:37 AM, libor.bukata@oracle.com wrote:
>> On non-Linux platforms, gdb crashes when compile command is issued
>> because of the null pointer in struct osabi_names gdb_osab. The attached
>> patch adds a check to avoid this crash and adds osabi name for Solaris.
>> However, there is probably more work required to enable compile feature
>> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
>>
> Thank you for the patch! The compile feature, as you have discovered, needs
> much more testing on non-linux configurations.
>
>> --- gdb-8.3/gdb/compile/compile.c	2019-08-19 13:07:57.669785758 +0000
>> +++ gdb-8.3/gdb/compile/compile.c	2019-08-19 13:07:33.865626973 +0000
>> @@ -697,7 +697,7 @@ compile_to_object (struct command_line *
>>         const char *arch_rx = gdbarch_gnu_triplet_regexp (gdbarch);
>>   
>>         /* Allow triplets with or without vendor set.  */
>> -      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + os_rx;
>> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
>>         compiler->set_triplet_regexp (triplet_rx.c_str ());
>>       }
> I'm not sure about this. Should os_rx be NULL (which you've shown is possible
> for a number of configurations), this would leave triplet_rx as $ARCH(-[^-]*)?-",
> e.g., "x86_64(-[^-]*)?-". I would call that a malformed regexp for this purpose.
>
> While the plugin may handle that gracefully, this just doesn't seem very
> user-friendly to me, but I am not a maintainer, so you should definitely
> wait for a real maintainer to chime in.
>
> Otherwise, the only comments I have relate to
>
>> +      triplet_rx = std::string (arch_rx) + "(-[^-]*)?-" + (os_rx ? : "");
> We prefer explicit comparisons with NULL/nullptr.
>
> Omitting the true-case expression is unusual. Not invalid, but certainly
> unusual (in gdb). I find no uses of this idiom in our sources. I would
> prefer to see the more explicit
>
>     os_rx != nullptr ? os_rx : ""
>
> but I'll let official maintainers chime in on this usage. I'm just as curious
> to see how others feel about it.
>
> Keith
  

Hi Libor,

> On non-Linux platforms, gdb crashes when compile command is issued
> because of the null pointer in struct osabi_names gdb_osab. The attached
> patch adds a check to avoid this crash and adds osabi name for Solaris.
> However, there is probably more work required to enable compile feature
> on Solaris (e.g., solaris_infcall_munmap) and other platforms.

just out of curiosity: what prompted you to try this?

I gave it a very quick whirl myself, trying to run the
gdb.compile/compile.exp test on amd64-pc-solaris2.11 with gdb master and
libcc1.so from gcc mainline: all I got was a SIGTRAP from the very first
compile command.

That said, I'm quite unlikely to work on this any time soon: with
ca. 2500 failures in the gdb testsuite on Solaris and even basic
features unimplented (e.g. I'm currently looking into TLS support), I
believe there are way more pressing issues.  However, if you want to
give it a try yourself, I'm more than happy to help get a patch in.

	Rainer
  

Hi Rainer,

I noticed it from the testsuite results (crash dump generated) for gdb 
8.3. Although it is not implemented for Solaris, it should not crash, 
which is the reason why I sent a patch that resolves a nullptr 
dereference. The patched gdb prints an error message (instead of 
crashing) and the user can continue with debugging. I agree with you 
that there many more pressing issues that needs to be resolved first.

Thanks,
Libor

On 9/9/19 10:07 AM, Rainer Orth wrote:
> Hi Libor,
>
>> On non-Linux platforms, gdb crashes when compile command is issued
>> because of the null pointer in struct osabi_names gdb_osab. The attached
>> patch adds a check to avoid this crash and adds osabi name for Solaris.
>> However, there is probably more work required to enable compile feature
>> on Solaris (e.g., solaris_infcall_munmap) and other platforms.
> just out of curiosity: what prompted you to try this?
>
> I gave it a very quick whirl myself, trying to run the
> gdb.compile/compile.exp test on amd64-pc-solaris2.11 with gdb master and
> libcc1.so from gcc mainline: all I got was a SIGTRAP from the very first
> compile command.
>
> That said, I'm quite unlikely to work on this any time soon: with
> ca. 2500 failures in the gdb testsuite on Solaris and even basic
> features unimplented (e.g. I'm currently looking into TLS support), I
> believe there are way more pressing issues.  However, if you want to
> give it a try yourself, I'm more than happy to help get a patch in.
>
> 	Rainer
>
  

>>>>> ">" == libor bukata <libor.bukata@oracle.com> writes:

>> I have no objections to use 'os_rx != NULL ? os_rx : ""' instead; it
>> is more portable. Note that NULL should be used instead of nullptr as
>> nullptr is available since C++11.

Just FYI, gdb uses C++11 nowadays, so nullptr is fine.

Tom
  

Hi Libor,

> I noticed it from the testsuite results (crash dump generated) for gdb
> 8.3. Although it is not implemented for Solaris, it should not crash, which
> is the reason why I sent a patch that resolves a nullptr dereference. The
> patched gdb prints an error message (instead of crashing) and the user can
> continue with debugging. I agree with you that there many more pressing
> issues that needs to be resolved first.

that explains why I'm not seeing this in my gdb builds: I use a
32-bit-default gcc 9.1 to build a 64-bit gdb, so there's no matching
libcc1.so around and the affected tests are UNTESTED.  Fully agreed that
there should be no crashes even for unsupported features, of course.

I'll leave the actual review to the responsible maintainers.

	Rainer