diff mbox series

libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD

Message ID 20231222023605.3894839-1-lipeng.zhu@intel.com
State Unresolved
Headers
Series libgfortran: Bugfix if not define HAVE_ATOMIC_FETCH_ADD |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Lipeng Zhu Dec. 22, 2023, 2:36 a.m. UTC 
  This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
not defined in dec_waiting_unlocked function.

libgfortran/ChangeLog:

	* io/io.h (dec_waiting_unlocked): Use
	__gthread_rwlock_wrlock/__gthread_rwlock_unlock or
	__gthread_mutex_lock/__gthread_mutex_unlock functions
	to replace WRLOCK and RWUNLOCK macros.

Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>
---
 libgfortran/io/io.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Tobias Burnus Jan. 3, 2024, 11:12 a.m. UTC | #1 
On 22.12.23 03:36, Lipeng Zhu wrote:
> This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
> not defined in dec_waiting_unlocked function.
>
> libgfortran/ChangeLog:
>
>       * io/io.h (dec_waiting_unlocked): Use
>       __gthread_rwlock_wrlock/__gthread_rwlock_unlock or
>       __gthread_mutex_lock/__gthread_mutex_unlock functions
>       to replace WRLOCK and RWUNLOCK macros.
>
> Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>

The change looks good to me + I assume it will work, but have not tested
it myself.

Downside is that it slightly breaks with the abstraction done with all
the macros, but it seems to be the simplest solution.

What is really missing - and should be included in the commit message
(before the ChangeLog block) - is the following information:

    As io.h does not include async.h, the WRLOCK and RWUNLOCK macros are
undefined.

(Or something similar in other words.)

I think that helps others when looking at "git log" and wondering *why*
that change was needed.

Thanks,

Tobias

>   libgfortran/io/io.h | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
> index 15daa0995b1..c7f0f7d7d9e 100644
> --- a/libgfortran/io/io.h
> +++ b/libgfortran/io/io.h
> @@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u)
>   #ifdef HAVE_ATOMIC_FETCH_ADD
>     (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
>   #else
> -  WRLOCK (&unit_rwlock);
> +#ifdef __GTHREAD_RWLOCK_INIT
> +  __gthread_rwlock_wrlock (&unit_rwlock);
> +  u->waiting--;
> +  __gthread_rwlock_unlock (&unit_rwlock);
> +#else
> +  __gthread_mutex_lock (&unit_rwlock);
>     u->waiting--;
> -  RWUNLOCK (&unit_rwlock);
> +  __gthread_mutex_unlock (&unit_rwlock);
> +#endif
>   #endif
>   }
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
Lipeng Zhu Jan. 4, 2024, 2:18 a.m. UTC | #2 
On 2024/1/3 19:12, Tobias Burnus wrote:
> On 22.12.23 03:36, Lipeng Zhu wrote:
>> This patch try to fix the bug when HAVE_ATOMIC_FETCH_ADD is
>> not defined in dec_waiting_unlocked function.
>>
>> libgfortran/ChangeLog:
>>
>>       * io/io.h (dec_waiting_unlocked): Use
>>       __gthread_rwlock_wrlock/__gthread_rwlock_unlock or
>>       __gthread_mutex_lock/__gthread_mutex_unlock functions
>>       to replace WRLOCK and RWUNLOCK macros.
>>
>> Signed-off-by: Lipeng Zhu <lipeng.zhu@intel.com>
> 
> The change looks good to me + I assume it will work, but have not tested
> it myself.
> 
> Downside is that it slightly breaks with the abstraction done with all
> the macros, but it seems to be the simplest solution.
> 

Hi Tobias,

Thanks for your review, the reason I changed like this is because I 
found when LOCK macro was first introduced in this patch: 
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=2b4c90656132abb8b8ad155d345c7d4fbf1687c9, 
it replaced __gthread_mutex_lock method with LOCK macro in other files 
like io/unit.c, but remained __gthread_mutex_lock in io/io.h. I am not 
sure if this is intentional or not, to avoid potential risk, I used the 
most straightforward solution.


> What is really missing - and should be included in the commit message
> (before the ChangeLog block) - is the following information:
> 
>     As io.h does not include async.h, the WRLOCK and RWUNLOCK macros are
> undefined.
> 
> (Or something similar in other words.)
> 
> I think that helps others when looking at "git log" and wondering *why*
> that change was needed.
> 
> Thanks,
> 
> Tobias
> 

Thanks, I will update the commit message as suggested.

Lipeng Zhu

>>   libgfortran/io/io.h | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
>> index 15daa0995b1..c7f0f7d7d9e 100644
>> --- a/libgfortran/io/io.h
>> +++ b/libgfortran/io/io.h
>> @@ -1020,9 +1020,15 @@ dec_waiting_unlocked (gfc_unit *u)
>>   #ifdef HAVE_ATOMIC_FETCH_ADD
>>     (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
>>   #else
>> -  WRLOCK (&unit_rwlock);
>> +#ifdef __GTHREAD_RWLOCK_INIT
>> +  __gthread_rwlock_wrlock (&unit_rwlock);
>> +  u->waiting--;
>> +  __gthread_rwlock_unlock (&unit_rwlock);
>> +#else
>> +  __gthread_mutex_lock (&unit_rwlock);
>>     u->waiting--;
>> -  RWUNLOCK (&unit_rwlock);
>> +  __gthread_mutex_unlock (&unit_rwlock);
>> +#endif
>>   #endif
>>   }
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 
> 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: 
> Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; 
> Registergericht München, HRB 106955
>
diff mbox series

Patch

diff --git a/libgfortran/io/io.h b/libgfortran/io/io.h
index 15daa0995b1..c7f0f7d7d9e 100644
--- a/libgfortran/io/io.h
+++ b/libgfortran/io/io.h
@@ -1020,9 +1020,15 @@  dec_waiting_unlocked (gfc_unit *u)
 #ifdef HAVE_ATOMIC_FETCH_ADD
   (void) __atomic_fetch_add (&u->waiting, -1, __ATOMIC_RELAXED);
 #else
-  WRLOCK (&unit_rwlock);
+#ifdef __GTHREAD_RWLOCK_INIT
+  __gthread_rwlock_wrlock (&unit_rwlock);
+  u->waiting--;
+  __gthread_rwlock_unlock (&unit_rwlock);
+#else
+  __gthread_mutex_lock (&unit_rwlock);
   u->waiting--;
-  RWUNLOCK (&unit_rwlock);
+  __gthread_mutex_unlock (&unit_rwlock);
+#endif
 #endif
 }