Feedback sought: patch to enable gdb to build on macOS Mojave

Feedback sought: patch to enable gdb to build on macOS Mojave

[Joubert Nel]

Hi, 

I worked through some issues to get gdb to build on macOS Mojave  

Specifically, I modified darwin-nat.c to overcome variable shadowing warnings (which get treated as errors by the build process). See point (b) at https://stackoverflow.com/questions/54736753/gdb-on-mac-mojave-during-startup-program-terminated-with-signal-unknown-sign

Worth submitting a patch so that others can easily build? 

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[Kevin Buettner]

On Mon, 18 Feb 2019 09:41:34 -0800
Joubert Nel <joubert@joubster.com> wrote:

> I worked through some issues to get gdb to build on macOS Mojave  
> 
> Specifically, I modified darwin-nat.c to overcome variable shadowing warnings (which get treated as errors by the build process). See point (b) at https://stackoverflow.com/questions/54736753/gdb-on-mac-mojave-during-startup-program-terminated-with-signal-unknown-sign
> 
> Worth submitting a patch so that others can easily build? 

We'd definitely like to have your shadowing fixes for darwin-nat.c
(plus fixes for anything else you ran into as well).

Kevin

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

>> Worth submitting a patch so that others can easily build? 

Kevin> We'd definitely like to have your shadowing fixes for darwin-nat.c
Kevin> (plus fixes for anything else you ran into as well).

+1 to what Kevin said.
Contribution instructions are here: https://sourceware.org/gdb/contribute/

Tom

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[Joubert Nel]

Great. 

I read the contribution checklist and I think those are all great guidelines. 

Before I prepare a formal submission email, it would be helpful to hear whether the actual code differences seem acceptable: https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed





> On Feb 18, 2019, at 2:47 PM, Tom Tromey <tom@tromey.com> wrote:
> 
>>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:
> 
>>> Worth submitting a patch so that others can easily build? 
> 
> Kevin> We'd definitely like to have your shadowing fixes for darwin-nat.c
> Kevin> (plus fixes for anything else you ran into as well).
> 
> +1 to what Kevin said.
> Contribution instructions are here: https://sourceware.org/gdb/contribute/
> 
> Tom

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[John Baldwin]

On 2/19/19 10:50 PM, Joubert Nel wrote:
> Great. 
> 
> I read the contribution checklist and I think those are all great guidelines. 
> 
> Before I prepare a formal submission email, it would be helpful to hear whether the actual code differences seem acceptable: https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed

Especially for a patch that small it is easier to review it inline.

I think the first hunk is fine.

The second one probably isn't the right fix.  The earlier 'res' is an
'int' for the return value from darwin_decode_notify_message.  The
later one is a pid_t for the return value from wait4.  I would perhaps
rename the second 'res' to 'wpid' instead of using the int.

For the third one it looks like you can just remove the line entirely.
priv is already set to 'get_darwin_inferior (inf)' at the start of the
function, so that assignment is redundant.

-- 
John Baldwin

                                                                            

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[Joubert Nel]

Thanks for the review, John & Simon. 

Your feedback makes sense and I’ve revised the suggested patch accordingly: https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed <https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed>



> On Feb 20, 2019, at 8:48 AM, John Baldwin <jhb@FreeBSD.org> wrote:
> 
> On 2/19/19 10:50 PM, Joubert Nel wrote:
>> Great. 
>> 
>> I read the contribution checklist and I think those are all great guidelines. 
>> 
>> Before I prepare a formal submission email, it would be helpful to hear whether the actual code differences seem acceptable: https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed
> 
> Especially for a patch that small it is easier to review it inline.
> 
> I think the first hunk is fine.
> 
> The second one probably isn't the right fix.  The earlier 'res' is an
> 'int' for the return value from darwin_decode_notify_message.  The
> later one is a pid_t for the return value from wait4.  I would perhaps
> rename the second 'res' to 'wpid' instead of using the int.
> 
> For the third one it looks like you can just remove the line entirely.
> priv is already set to 'get_darwin_inferior (inf)' at the start of the
> function, so that assignment is redundant.
> 
> -- 
> John Baldwin
> 
>                                                                             

Re: Feedback sought: patch to enable gdb to build on macOS Mojave

[John Baldwin]

On 2/23/19 3:14 PM, Joubert Nel wrote:
> Thanks for the review, John & Simon. 
> 
> Your feedback makes sense and I’ve revised the suggested patch accordingly: https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed <https://gist.github.com/joubertnel/267ca0fff4eaad494cc19ec3ba7953ed>

I think this looks fine, but you should send it as a normal patch as Kevin
and Tom noted previously.

-- 
John Baldwin