diff mbox series

uqmi: fix compilation with GCC12

Message ID 20220609014715.1353-1-rosenp@gmail.com
State New
Headers show
Series uqmi: fix compilation with GCC12 | expand

Commit Message

Rosen Penev June 9, 2022, 1:47 a.m. UTC 
GCC12 doesn't seem to see that the completed member gets nulled. Use
malloc to work around this.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 commands-nas.c | 2 +-
 dev.c          | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

e9hack June 11, 2022, 7:04 a.m. UTC | #1 
The 'dangling pointer' issue can be fix without using malloc().

--- a/dev.c	2022-05-04 02:18:17.000000000 +0200
+++ b/dev.c	2022-06-11 08:48:21.185567953 +0200
@@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
  int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
  {
  	bool complete = false;
+	bool *c;
  	bool cancelled;
  
  	if (!req->pending)
@@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
  		uloop_cancelled = cancelled;
  	}
  
-	if (req->complete == &complete)
-		req->complete = NULL;
+	c = req->complete;
+	req->complete = NULL;
+	if (c != &complete)
+		req->complete = c;
  
  	return req->ret;
  }


Am 09.06.2022 um 03:47 schrieb Rosen Penev:
> GCC12 doesn't seem to see that the completed member gets nulled. Use
> malloc to work around this.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>   commands-nas.c | 2 +-
>   dev.c          | 9 +++++----
>   2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/commands-nas.c b/commands-nas.c
> index 476cd61..9c0a626 100644
> --- a/commands-nas.c
> +++ b/commands-nas.c
> @@ -710,7 +710,7 @@ static void
>   cmd_nas_get_cell_location_info_cb(struct qmi_dev *qmi, struct qmi_request *req, struct qmi_msg *msg)
>   {
>   	struct qmi_nas_get_cell_location_info_response res;
> -	void *c, *t, *cell, *freq;
> +	void *c = NULL, *t, *cell, *freq;
>   	int i, j;
>   
>   	qmi_parse_nas_get_cell_location_info_response(msg, &res);
> diff --git a/dev.c b/dev.c
> index bd10207..b1cf2a3 100644
> --- a/dev.c
> +++ b/dev.c
> @@ -205,7 +205,7 @@ void qmi_request_cancel(struct qmi_dev *qmi, struct qmi_request *req)
>   
>   int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>   {
> -	bool complete = false;
> +	bool *complete = malloc(sizeof(bool));
>   	bool cancelled;
>   
>   	if (!req->pending)
> @@ -214,8 +214,8 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>   	if (req->complete)
>   		*req->complete = true;
>   
> -	req->complete = &complete;
> -	while (!complete) {
> +	req->complete = complete;
> +	while (!*complete) {
>   		cancelled = uloop_cancelled;
>   		uloop_cancelled = false;
>   		uloop_run();
> @@ -226,9 +226,10 @@ int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>   		uloop_cancelled = cancelled;
>   	}
>   
> -	if (req->complete == &complete)
> +	if (req->complete == complete)
>   		req->complete = NULL;
>   
> +	free(complete);
>   	return req->ret;
>   }
>
Bjørn Mork June 11, 2022, 2:47 p.m. UTC | #2 
e9hack <e9hack@gmail.com> writes:

> The 'dangling pointer' issue can be fix without using malloc().
>
> --- a/dev.c	2022-05-04 02:18:17.000000000 +0200
> +++ b/dev.c	2022-06-11 08:48:21.185567953 +0200
> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
>  int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>  {
>  	bool complete = false;
> +	bool *c;
>  	bool cancelled;
>    	if (!req->pending)
> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
>  		uloop_cancelled = cancelled;
>  	}
>  -	if (req->complete == &complete)
> -		req->complete = NULL;
> +	c = req->complete;
> +	req->complete = NULL;
> +	if (c != &complete)
> +		req->complete = c;
>    	return req->ret;
>  }

How about just fixing GCC instead?  Having all sorts of funny and
pointless code just to avoid bogus compiler warnings is kind of stupid,
isn't it?

If GCC can't do better that this, then it's much better to disable that
warning.


Bjørn
Hauke Mehrtens Aug. 21, 2022, 2:54 p.m. UTC | #3 
On 6/11/22 16:47, Bjørn Mork wrote:
> e9hack <e9hack@gmail.com> writes:
> 
>> The 'dangling pointer' issue can be fix without using malloc().
>>
>> --- a/dev.c	2022-05-04 02:18:17.000000000 +0200
>> +++ b/dev.c	2022-06-11 08:48:21.185567953 +0200
>> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
>>   int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>>   {
>>   	bool complete = false;
>> +	bool *c;
>>   	bool cancelled;
>>     	if (!req->pending)
>> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
>>   		uloop_cancelled = cancelled;
>>   	}
>>   -	if (req->complete == &complete)
>> -		req->complete = NULL;
>> +	c = req->complete;
>> +	req->complete = NULL;
>> +	if (c != &complete)
>> +		req->complete = c;
>>     	return req->ret;
>>   }
> 
> How about just fixing GCC instead?  Having all sorts of funny and
> pointless code just to avoid bogus compiler warnings is kind of stupid,
> isn't it?
> 
> If GCC can't do better that this, then it's much better to disable that
> warning.

GCC complains about this code because the pointer is only removed 
conditionally
-------------------------------------------
	if (req->complete == &complete)
		req->complete = NULL;
-------------------------------------------
https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229

When you make this part unconditionally it does not complain any more.

Is it possible that something changes the req->complete pointer address 
in between?

Hauke
Bjørn Mork Aug. 21, 2022, 3:02 p.m. UTC | #4 
Hauke Mehrtens <hauke@hauke-m.de> writes:

> GCC complains about this code because the pointer is only removed
> conditionally
> -------------------------------------------
> 	if (req->complete == &complete)
> 		req->complete = NULL;
> -------------------------------------------
> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229


Yes, but that condition is testing for precisely the dangling
pointer.GCC complains about.  If it is false, then there is no
problem. If it is true, then the pointer is removed and there is no
problem.

If GCC is unable to figure that out, then it should not try.


Bjørn
Rosen Penev Sept. 28, 2022, 2:22 a.m. UTC | #5 
On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote:
>
> On 6/11/22 16:47, Bjørn Mork wrote:
> > e9hack <e9hack@gmail.com> writes:
> >
> >> The 'dangling pointer' issue can be fix without using malloc().
> >>
> >> --- a/dev.c  2022-05-04 02:18:17.000000000 +0200
> >> +++ b/dev.c  2022-06-11 08:48:21.185567953 +0200
> >> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
> >>   int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
> >>   {
> >>      bool complete = false;
> >> +    bool *c;
> >>      bool cancelled;
> >>      if (!req->pending)
> >> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
> >>              uloop_cancelled = cancelled;
> >>      }
> >>   -  if (req->complete == &complete)
> >> -            req->complete = NULL;
> >> +    c = req->complete;
> >> +    req->complete = NULL;
> >> +    if (c != &complete)
> >> +            req->complete = c;
> >>      return req->ret;
> >>   }
> >
> > How about just fixing GCC instead?  Having all sorts of funny and
> > pointless code just to avoid bogus compiler warnings is kind of stupid,
> > isn't it?
> >
> > If GCC can't do better that this, then it's much better to disable that
> > warning.
>
> GCC complains about this code because the pointer is only removed
> conditionally
> -------------------------------------------
>         if (req->complete == &complete)
>                 req->complete = NULL;
> -------------------------------------------
> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229
>
> When you make this part unconditionally it does not complain any more.
>
> Is it possible that something changes the req->complete pointer address
> in between?
this is still an issue.
>
> Hauke
Hauke Mehrtens Sept. 29, 2022, 2:46 p.m. UTC | #6 
On 9/28/22 04:22, Rosen Penev wrote:
> On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote:
>>
>> On 6/11/22 16:47, Bjørn Mork wrote:
>>> e9hack <e9hack@gmail.com> writes:
>>>
>>>> The 'dangling pointer' issue can be fix without using malloc().
>>>>
>>>> --- a/dev.c  2022-05-04 02:18:17.000000000 +0200
>>>> +++ b/dev.c  2022-06-11 08:48:21.185567953 +0200
>>>> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
>>>>    int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
>>>>    {
>>>>       bool complete = false;
>>>> +    bool *c;
>>>>       bool cancelled;
>>>>       if (!req->pending)
>>>> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
>>>>               uloop_cancelled = cancelled;
>>>>       }
>>>>    -  if (req->complete == &complete)
>>>> -            req->complete = NULL;
>>>> +    c = req->complete;
>>>> +    req->complete = NULL;
>>>> +    if (c != &complete)
>>>> +            req->complete = c;
>>>>       return req->ret;
>>>>    }
>>>
>>> How about just fixing GCC instead?  Having all sorts of funny and
>>> pointless code just to avoid bogus compiler warnings is kind of stupid,
>>> isn't it?
>>>
>>> If GCC can't do better that this, then it's much better to disable that
>>> warning.
>>
>> GCC complains about this code because the pointer is only removed
>> conditionally
>> -------------------------------------------
>>          if (req->complete == &complete)
>>                  req->complete = NULL;
>> -------------------------------------------
>> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229
>>
>> When you make this part unconditionally it does not complain any more.
>>
>> Is it possible that something changes the req->complete pointer address
>> in between?
> this is still an issue.
>>
>> Hauke
Hi,

Is it possible to deactivate the error and make it only a warning which 
we can ignore?
I would deactivate this error for the complete application and add a 
comment to it that this is a workaround for GCC 12.

Did someone create a ticket at GCC for this problem? I am aware of the 
one for elfutils only.

Hauke
Rosen Penev Sept. 29, 2022, 9:32 p.m. UTC | #7 
On Thu, Sep 29, 2022 at 7:45 AM Hauke Mehrtens <hauke@hauke-m.de> wrote:
>
> On 9/28/22 04:22, Rosen Penev wrote:
> > On Sun, Aug 21, 2022 at 7:54 AM Hauke Mehrtens <hauke@hauke-m.de> wrote:
> >>
> >> On 6/11/22 16:47, Bjørn Mork wrote:
> >>> e9hack <e9hack@gmail.com> writes:
> >>>
> >>>> The 'dangling pointer' issue can be fix without using malloc().
> >>>>
> >>>> --- a/dev.c  2022-05-04 02:18:17.000000000 +0200
> >>>> +++ b/dev.c  2022-06-11 08:48:21.185567953 +0200
> >>>> @@ -206,6 +206,7 @@ void qmi_request_cancel(struct qmi_dev *
> >>>>    int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
> >>>>    {
> >>>>       bool complete = false;
> >>>> +    bool *c;
> >>>>       bool cancelled;
> >>>>       if (!req->pending)
> >>>> @@ -226,8 +227,10 @@ int qmi_request_wait(struct qmi_dev *qmi
> >>>>               uloop_cancelled = cancelled;
> >>>>       }
> >>>>    -  if (req->complete == &complete)
> >>>> -            req->complete = NULL;
> >>>> +    c = req->complete;
> >>>> +    req->complete = NULL;
> >>>> +    if (c != &complete)
> >>>> +            req->complete = c;
> >>>>       return req->ret;
> >>>>    }
> >>>
> >>> How about just fixing GCC instead?  Having all sorts of funny and
> >>> pointless code just to avoid bogus compiler warnings is kind of stupid,
> >>> isn't it?
> >>>
> >>> If GCC can't do better that this, then it's much better to disable that
> >>> warning.
> >>
> >> GCC complains about this code because the pointer is only removed
> >> conditionally
> >> -------------------------------------------
> >>          if (req->complete == &complete)
> >>                  req->complete = NULL;
> >> -------------------------------------------
> >> https://git.openwrt.org/?p=project/uqmi.git;a=blob;f=dev.c;h=bd1020790f844fd364fd753135acd8f53f34d996;hb=HEAD#l229
> >>
> >> When you make this part unconditionally it does not complain any more.
> >>
> >> Is it possible that something changes the req->complete pointer address
> >> in between?
> > this is still an issue.
> >>
> >> Hauke
> Hi,
>
> Is it possible to deactivate the error and make it only a warning which
> we can ignore?
> I would deactivate this error for the complete application and add a
> comment to it that this is a workaround for GCC 12.
>
> Did someone create a ticket at GCC for this problem? I am aware of the
> one for elfutils only.
What's wrong with working around it? It's not difficult.
>
> Hauke
diff mbox series

Patch

diff --git a/commands-nas.c b/commands-nas.c
index 476cd61..9c0a626 100644
--- a/commands-nas.c
+++ b/commands-nas.c
@@ -710,7 +710,7 @@  static void
 cmd_nas_get_cell_location_info_cb(struct qmi_dev *qmi, struct qmi_request *req, struct qmi_msg *msg)
 {
 	struct qmi_nas_get_cell_location_info_response res;
-	void *c, *t, *cell, *freq;
+	void *c = NULL, *t, *cell, *freq;
 	int i, j;
 
 	qmi_parse_nas_get_cell_location_info_response(msg, &res);
diff --git a/dev.c b/dev.c
index bd10207..b1cf2a3 100644
--- a/dev.c
+++ b/dev.c
@@ -205,7 +205,7 @@  void qmi_request_cancel(struct qmi_dev *qmi, struct qmi_request *req)
 
 int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
 {
-	bool complete = false;
+	bool *complete = malloc(sizeof(bool));
 	bool cancelled;
 
 	if (!req->pending)
@@ -214,8 +214,8 @@  int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
 	if (req->complete)
 		*req->complete = true;
 
-	req->complete = &complete;
-	while (!complete) {
+	req->complete = complete;
+	while (!*complete) {
 		cancelled = uloop_cancelled;
 		uloop_cancelled = false;
 		uloop_run();
@@ -226,9 +226,10 @@  int qmi_request_wait(struct qmi_dev *qmi, struct qmi_request *req)
 		uloop_cancelled = cancelled;
 	}
 
-	if (req->complete == &complete)
+	if (req->complete == complete)
 		req->complete = NULL;
 
+	free(complete);
 	return req->ret;
 }