From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x32f.google.com (mail-wm1-x32f.google.com [IPv6:2a00:1450:4864:20::32f]) by sourceware.org (Postfix) with ESMTPS id 122D63858C52 for ; Mon, 20 May 2024 07:49:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 122D63858C52 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 122D63858C52 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::32f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716191368; cv=none; b=uN6XcW8/z0m+Hl1i3vwIeU5T1jT+HM8SDuC/5WF7MSwnJxtH+1/7Bo3zBKA01R00jzfW5mTAKjjlLwwRRiRe8C3leqant5eEkelWtGmnkWG+DuRFGrzL79pEPRfGXKscMpv61flK69gApBxPz7x3vQY8uwJKO/4y/72A3gcU3Ag= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716191368; c=relaxed/simple; bh=8HeEeOuIIqO0NQxY4pKYMg5vTgWREW8HT//0OCgkfWQ=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=VGBUZ6KUQZ12h9ak7JXKL6+JLT28lCF5+AMXio/U7oz/oVDi3sr6C7VGcA4vMiOFKMsGLBEPvr8yOnr34wZCN2SA4R9B3qqI1UZeiNRoMKlyBdN5mkjhbyLIpDimE2whF5EnRdEHbUtU1YaiFbkwhayq/xeWkDG9JodYeO1A8oo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wm1-x32f.google.com with SMTP id 5b1f17b1804b1-420180b58c3so17906215e9.2 for ; Mon, 20 May 2024 00:49:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1716191363; x=1716796163; darn=gcc.gnu.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=7cmcfNJcnmx94xXeTdzXwHQGLPgF1rdhChdtIWXuaAw=; b=YDeYWVki7S71zh+QvKVb4ptCoO8erlY2awK8Gf6rSR2Makb06Ha3az9bokE0O9kH8W HJ/W8GXPLiUYJZEJg7s40rZP5pr4p7I7LM4JGEBX2UkDX2WZN6Gcw9l4tmwEHtege2WU qq8ajRF93KcVxawLJddywD3THXwoPQWyR9geTeybKAa9oDIIESKFq9L+tqLokhFQIwmU IWpaBpt8HUPnJhr6+F348zG/JoWKwxU7oNTi9hZ03lJQRb3O39hxetBxp8wxcvFdIzQB H8e4erex0XRMyBANdXjSdB2DMeeFjFus49mWoiIHTSLcylDmytmSe1WNmQDgbqEDh22K A2og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716191363; x=1716796163; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7cmcfNJcnmx94xXeTdzXwHQGLPgF1rdhChdtIWXuaAw=; b=bJcjMmBxhm7lSGDS0jRLyV1nhTtEnN9KDAPAUKKGnpqJqiBM5w0gsNxwf2W5oOLhcR QiVLZe0SOt6D5dqMIIA3slDfJPrzIZna6gwud1XnMW61n5+pz9RKfAWNgBbt+zGxZ85Y X0z5xm6KBkvy6kuLAX7C17QqCq+KlBOB6N2T4QIx8V4sRO0+qtGNnvGnd4cjlrt/65wr q+EjsnXmPEJCHrmKC0tSlcqYdH1494n7ahBI8kLg2+5Wq61Ttnm+ezJmQ5ZGaZsRGcOk TTWx5gsHRtjV2tHDNzZ79x9ZvBbXcDmqhDToUpoffxqxp+/Tz0O5g8ocvV8YxlQn3mEF qwvw== X-Gm-Message-State: AOJu0YxUZRZRSJPpZxVIqG5pGBKcUBwIa2rvFGuukbIHiCc+JMicgHEQ /+OXpLflY03MjxjDNV8FuKQlqcE8iZijsG/XrAKrfD/TuGdvTEQDoVaESnnPSPaUfs6IbH02tw8 = X-Google-Smtp-Source: AGHT+IFlB+GQzolK+6tyB/CqwIFHtQirk/+/YE1b8TLSfYaShOtbOUWHV9ryraPAqaGmi12u0hz2Qw== X-Received: by 2002:a05:600c:474e:b0:420:1189:e14c with SMTP id 5b1f17b1804b1-4201189e319mr165381465e9.40.1716191362604; Mon, 20 May 2024 00:49:22 -0700 (PDT) Received: from poulhies-Precision-5550.lan ([2001:861:3382:1a90:de37:8b1c:1f33:2610]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-41f88111033sm446892175e9.34.2024.05.20.00.49.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 May 2024 00:49:22 -0700 (PDT) From: =?UTF-8?q?Marc=20Poulhi=C3=A8s?= To: gcc-patches@gcc.gnu.org Cc: Ronan Desplanques Subject: [COMMITTED 14/30] ada: Tweak handling of thread ID on POSIX Date: Mon, 20 May 2024 09:48:40 +0200 Message-ID: <20240520074858.222435-14-poulhies@adacore.com> X-Mailer: git-send-email 2.43.2 In-Reply-To: <20240520074858.222435-1-poulhies@adacore.com> References: <20240520074858.222435-1-poulhies@adacore.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: From: Ronan Desplanques This patch changes the task initialization subprograms on POSIX platforms so that the thread ID of an ATCB is only set once. This has the advantage of getting rid of the Atomic aspect on the corresponding record component, and silences a Helgrind warning about a data race. gcc/ada/ * libgnarl/s-taprop__linux.adb (Enter_Task): Move setting of thread ID out of Enter_Task. (Initialize): Set thread ID for the environment task. (Create_Task): Remove now unnecessary Unrestricted_Access attribute and add justification for a memory write. * libgnarl/s-taprop__posix.adb: Likewise. * libgnarl/s-taprop__qnx.adb: Likewise. * libgnarl/s-taprop__rtems.adb: Likewise. * libgnarl/s-taprop__solaris.adb: Likewise. * libgnarl/s-taspri__posix.ads: Remove pragma Atomic for Private_Data.Thread, and update documentation comment. * libgnarl/s-taspri__lynxos.ads: Likewise. * libgnarl/s-taspri__posix-noaltstack.ads: Likewise. * libgnarl/s-taspri__solaris.ads: Likewise. * libgnarl/s-tporft.adb (Register_Foreign_Thread): Adapt to Enter_Task not setting the thread ID anymore. * libgnarl/s-tassta.adb (Task_Wrapper): Update comment. Tested on x86_64-pc-linux-gnu, committed on master. --- gcc/ada/libgnarl/s-taprop__linux.adb | 14 +++++++------- gcc/ada/libgnarl/s-taprop__posix.adb | 14 +++++++------- gcc/ada/libgnarl/s-taprop__qnx.adb | 14 +++++++------- gcc/ada/libgnarl/s-taprop__rtems.adb | 14 +++++++------- gcc/ada/libgnarl/s-taprop__solaris.adb | 16 ++++++++-------- gcc/ada/libgnarl/s-taspri__lynxos.ads | 16 ++++++++++------ gcc/ada/libgnarl/s-taspri__posix-noaltstack.ads | 16 ++++++++++------ gcc/ada/libgnarl/s-taspri__posix.ads | 16 ++++++++++------ gcc/ada/libgnarl/s-taspri__solaris.ads | 16 ++++++++++------ gcc/ada/libgnarl/s-tassta.adb | 2 +- gcc/ada/libgnarl/s-tporft.adb | 1 + 11 files changed, 78 insertions(+), 61 deletions(-) diff --git a/gcc/ada/libgnarl/s-taprop__linux.adb b/gcc/ada/libgnarl/s-taprop__linux.adb index 0c09817739c..0a51b3601c0 100644 --- a/gcc/ada/libgnarl/s-taprop__linux.adb +++ b/gcc/ada/libgnarl/s-taprop__linux.adb @@ -730,7 +730,6 @@ package body System.Task_Primitives.Operations is raise Invalid_CPU_Number; end if; - Self_ID.Common.LL.Thread := pthread_self; Self_ID.Common.LL.LWP := lwp_self; -- Set thread name to ease debugging. If the name of the task is @@ -1004,14 +1003,14 @@ package body System.Task_Primitives.Operations is -- do not need to manipulate caller's signal mask at this point. -- All tasks in RTS will have All_Tasks_Mask initially. - -- Note: the use of Unrestricted_Access in the following call is needed - -- because otherwise we have an error of getting a access-to-volatile - -- value which points to a non-volatile object. But in this case it is - -- safe to do this, since we know we have no problems with aliasing and - -- Unrestricted_Access bypasses this check. + -- The write to T.Common.LL.Thread is not racy with regard to the + -- created thread because the created thread will not access it until + -- we release the RTS lock (or the current task's lock when + -- Restricted.Stages is used). One can verify that by inspecting the + -- Task_Wrapper procedures. Result := pthread_create - (T.Common.LL.Thread'Unrestricted_Access, + (T.Common.LL.Thread'Access, Thread_Attr'Access, Thread_Body_Access (Wrapper), To_Address (T)); @@ -1385,6 +1384,7 @@ package body System.Task_Primitives.Operations is begin Environment_Task_Id := Environment_Task; + Environment_Task.Common.LL.Thread := pthread_self; Interrupt_Management.Initialize; diff --git a/gcc/ada/libgnarl/s-taprop__posix.adb b/gcc/ada/libgnarl/s-taprop__posix.adb index 7ed52ea2d82..fb70aaf4976 100644 --- a/gcc/ada/libgnarl/s-taprop__posix.adb +++ b/gcc/ada/libgnarl/s-taprop__posix.adb @@ -636,7 +636,6 @@ package body System.Task_Primitives.Operations is procedure Enter_Task (Self_ID : Task_Id) is begin - Self_ID.Common.LL.Thread := pthread_self; Self_ID.Common.LL.LWP := lwp_self; Specific.Set (Self_ID); @@ -841,14 +840,14 @@ package body System.Task_Primitives.Operations is -- do not need to manipulate caller's signal mask at this point. -- All tasks in RTS will have All_Tasks_Mask initially. - -- Note: the use of Unrestricted_Access in the following call is needed - -- because otherwise we have an error of getting a access-to-volatile - -- value which points to a non-volatile object. But in this case it is - -- safe to do this, since we know we have no problems with aliasing and - -- Unrestricted_Access bypasses this check. + -- The write to T.Common.LL.Thread is not racy with regard to the + -- created thread because the created thread will not access it until + -- we release the RTS lock (or the current task's lock when + -- Restricted.Stages is used). One can verify that by inspecting the + -- Task_Wrapper procedures. Result := pthread_create - (T.Common.LL.Thread'Unrestricted_Access, + (T.Common.LL.Thread'Access, Attributes'Access, Thread_Body_Access (Wrapper), To_Address (T)); @@ -1260,6 +1259,7 @@ package body System.Task_Primitives.Operations is begin Environment_Task_Id := Environment_Task; + Environment_Task.Common.LL.Thread := pthread_self; Interrupt_Management.Initialize; diff --git a/gcc/ada/libgnarl/s-taprop__qnx.adb b/gcc/ada/libgnarl/s-taprop__qnx.adb index 108180d0617..f475c05c562 100644 --- a/gcc/ada/libgnarl/s-taprop__qnx.adb +++ b/gcc/ada/libgnarl/s-taprop__qnx.adb @@ -654,7 +654,6 @@ package body System.Task_Primitives.Operations is procedure Enter_Task (Self_ID : Task_Id) is begin - Self_ID.Common.LL.Thread := pthread_self; Self_ID.Common.LL.LWP := lwp_self; Specific.Set (Self_ID); @@ -846,14 +845,14 @@ package body System.Task_Primitives.Operations is -- do not need to manipulate caller's signal mask at this point. -- All tasks in RTS will have All_Tasks_Mask initially. - -- Note: the use of Unrestricted_Access in the following call is needed - -- because otherwise we have an error of getting a access-to-volatile - -- value which points to a non-volatile object. But in this case it is - -- safe to do this, since we know we have no problems with aliasing and - -- Unrestricted_Access bypasses this check. + -- The write to T.Common.LL.Thread is not racy with regard to the + -- created thread because the created thread will not access it until + -- we release the RTS lock (or the current task's lock when + -- Restricted.Stages is used). One can verify that by inspecting the + -- Task_Wrapper procedures. Result := pthread_create - (T.Common.LL.Thread'Unrestricted_Access, + (T.Common.LL.Thread'Access, Attributes'Access, Thread_Body_Access (Wrapper), To_Address (T)); @@ -1261,6 +1260,7 @@ package body System.Task_Primitives.Operations is begin Environment_Task_Id := Environment_Task; + Environment_Task.Common.LL.Thread := pthread_self; Interrupt_Management.Initialize; diff --git a/gcc/ada/libgnarl/s-taprop__rtems.adb b/gcc/ada/libgnarl/s-taprop__rtems.adb index 3feafd8bc3a..ea8422cb454 100644 --- a/gcc/ada/libgnarl/s-taprop__rtems.adb +++ b/gcc/ada/libgnarl/s-taprop__rtems.adb @@ -646,7 +646,6 @@ package body System.Task_Primitives.Operations is procedure Enter_Task (Self_ID : Task_Id) is begin - Self_ID.Common.LL.Thread := pthread_self; Self_ID.Common.LL.LWP := lwp_self; Specific.Set (Self_ID); @@ -851,14 +850,14 @@ package body System.Task_Primitives.Operations is -- do not need to manipulate caller's signal mask at this point. -- All tasks in RTS will have All_Tasks_Mask initially. - -- Note: the use of Unrestricted_Access in the following call is needed - -- because otherwise we have an error of getting a access-to-volatile - -- value which points to a non-volatile object. But in this case it is - -- safe to do this, since we know we have no problems with aliasing and - -- Unrestricted_Access bypasses this check. + -- The write to T.Common.LL.Thread is not racy with regard to the + -- created thread because the created thread will not access it until + -- we release the RTS lock (or the current task's lock when + -- Restricted.Stages is used). One can verify that by inspecting the + -- Task_Wrapper procedures. Result := pthread_create - (T.Common.LL.Thread'Unrestricted_Access, + (T.Common.LL.Thread'Access, Attributes'Access, Thread_Body_Access (Wrapper), To_Address (T)); @@ -1270,6 +1269,7 @@ package body System.Task_Primitives.Operations is begin Environment_Task_Id := Environment_Task; + Environment_Task.Common.LL.Thread := pthread_self; Interrupt_Management.Initialize; diff --git a/gcc/ada/libgnarl/s-taprop__solaris.adb b/gcc/ada/libgnarl/s-taprop__solaris.adb index 82e51b8d25c..09f90e6e204 100644 --- a/gcc/ada/libgnarl/s-taprop__solaris.adb +++ b/gcc/ada/libgnarl/s-taprop__solaris.adb @@ -424,6 +424,7 @@ package body System.Task_Primitives.Operations is begin Environment_Task_Id := Environment_Task; + Self_ID.Common.LL.Thread := thr_self; Interrupt_Management.Initialize; @@ -868,8 +869,7 @@ package body System.Task_Primitives.Operations is procedure Enter_Task (Self_ID : Task_Id) is begin - Self_ID.Common.LL.Thread := thr_self; - Self_ID.Common.LL.LWP := lwp_self; + Self_ID.Common.LL.LWP := lwp_self; Set_Task_Affinity (Self_ID); Specific.Set (Self_ID); @@ -997,11 +997,11 @@ package body System.Task_Primitives.Operations is Opts := THR_DETACHED + THR_BOUND; end if; - -- Note: the use of Unrestricted_Access in the following call is needed - -- because otherwise we have an error of getting a access-to-volatile - -- value which points to a non-volatile object. But in this case it is - -- safe to do this, since we know we have no problems with aliasing and - -- Unrestricted_Access bypasses this check. + -- The write to T.Common.LL.Thread is not racy with regard to the + -- created thread because the created thread will not access it until + -- we release the RTS lock (or the current task's lock when + -- Restricted.Stages is used). One can verify that by inspecting the + -- Task_Wrapper procedures. Result := thr_create @@ -1010,7 +1010,7 @@ package body System.Task_Primitives.Operations is Thread_Body_Access (Wrapper), To_Address (T), Opts, - T.Common.LL.Thread'Unrestricted_Access); + T.Common.LL.Thread'Access); Succeeded := Result = 0; pragma Assert diff --git a/gcc/ada/libgnarl/s-taspri__lynxos.ads b/gcc/ada/libgnarl/s-taspri__lynxos.ads index a3307000c80..f5e434eada6 100644 --- a/gcc/ada/libgnarl/s-taspri__lynxos.ads +++ b/gcc/ada/libgnarl/s-taspri__lynxos.ads @@ -86,12 +86,16 @@ private type Private_Data is limited record Thread : aliased System.OS_Interface.pthread_t; - pragma Atomic (Thread); - -- Thread field may be updated by two different threads of control. - -- (See, Enter_Task and Create_Task in s-taprop.adb). They put the same - -- value (thr_self value). We do not want to use lock on those - -- operations and the only thing we have to make sure is that they are - -- updated in atomic fashion. + -- This component is written to once before concurrent access to it is + -- possible, and then remains constant. The place where it is written to + -- depends on how the enclosing ATCB comes into existence: + -- + -- 1. For the environment task, the component is set in + -- System.Task_Primitive.Operations.Initialize. + -- 2. For foreign threads, it happens in + -- System.Task_Primitives.Operations.Register_Foreign_Thread. + -- 3. For others tasks, it's in + -- System.Task_Primitives.Operations.Create_Task. LWP : aliased System.OS_Interface.pthread_t; -- The purpose of this field is to provide a better tasking support on diff --git a/gcc/ada/libgnarl/s-taspri__posix-noaltstack.ads b/gcc/ada/libgnarl/s-taspri__posix-noaltstack.ads index b92f1dd4ab2..fb7e07d10cd 100644 --- a/gcc/ada/libgnarl/s-taspri__posix-noaltstack.ads +++ b/gcc/ada/libgnarl/s-taspri__posix-noaltstack.ads @@ -89,12 +89,16 @@ private type Private_Data is limited record Thread : aliased System.OS_Interface.pthread_t; - pragma Atomic (Thread); - -- Thread field may be updated by two different threads of control. - -- (See, Enter_Task and Create_Task in s-taprop.adb). They put the same - -- value (thr_self value). We do not want to use lock on those - -- operations and the only thing we have to make sure is that they are - -- updated in atomic fashion. + -- This component is written to once before concurrent access to it is + -- possible, and then remains constant. The place where it is written to + -- depends on how the enclosing ATCB comes into existence: + -- + -- 1. For the environment task, the component is set in + -- System.Task_Primitive.Operations.Initialize. + -- 2. For foreign threads, it happens in + -- System.Task_Primitives.Operations.Register_Foreign_Thread. + -- 3. For others tasks, it's in + -- System.Task_Primitives.Operations.Create_Task. LWP : aliased System.Address; -- The purpose of this field is to provide a better tasking support on diff --git a/gcc/ada/libgnarl/s-taspri__posix.ads b/gcc/ada/libgnarl/s-taspri__posix.ads index 4d0b379556d..3453f4fea4c 100644 --- a/gcc/ada/libgnarl/s-taspri__posix.ads +++ b/gcc/ada/libgnarl/s-taspri__posix.ads @@ -88,12 +88,16 @@ private type Private_Data is limited record Thread : aliased System.OS_Interface.pthread_t; - pragma Atomic (Thread); - -- Thread field may be updated by two different threads of control. - -- (See, Enter_Task and Create_Task in s-taprop.adb). They put the same - -- value (thr_self value). We do not want to use lock on those - -- operations and the only thing we have to make sure is that they are - -- updated in atomic fashion. + -- This component is written to once before concurrent access to it is + -- possible, and then remains constant. The place where it is written to + -- depends on how the enclosing ATCB comes into existence: + -- + -- 1. For the environment task, the component is set in + -- System.Task_Primitive.Operations.Initialize. + -- 2. For foreign threads, it happens in + -- System.Task_Primitives.Operations.Register_Foreign_Thread. + -- 3. For others tasks, it's in + -- System.Task_Primitives.Operations.Create_Task. LWP : aliased System.Address; -- The purpose of this field is to provide a better tasking support on diff --git a/gcc/ada/libgnarl/s-taspri__solaris.ads b/gcc/ada/libgnarl/s-taspri__solaris.ads index 16fc4196b00..586c971dce6 100644 --- a/gcc/ada/libgnarl/s-taspri__solaris.ads +++ b/gcc/ada/libgnarl/s-taspri__solaris.ads @@ -95,12 +95,16 @@ private type Private_Data is limited record Thread : aliased System.OS_Interface.thread_t; - pragma Atomic (Thread); - -- Thread field may be updated by two different threads of control. - -- (See, Enter_Task and Create_Task in s-taprop.adb). They put the same - -- value (thr_self value). We do not want to use lock on those - -- operations and the only thing we have to make sure is that they are - -- updated in atomic fashion. + -- This component is written to once before concurrent access to it is + -- possible, and then remains constant. The place where it is written to + -- depends on how the enclosing ATCB comes into existence: + -- + -- 1. For the environment task, the component is set in + -- System.Task_Primitive.Operations.Initialize. + -- 2. For foreign threads, it happens in + -- System.Task_Primitives.Operations.Register_Foreign_Thread. + -- 3. For others tasks, it's in + -- System.Task_Primitives.Operations.Create_Task. LWP : System.OS_Interface.lwpid_t; -- The LWP id of the thread. Set by self in Enter_Task diff --git a/gcc/ada/libgnarl/s-tassta.adb b/gcc/ada/libgnarl/s-tassta.adb index 01c94b950ba..594a1672866 100644 --- a/gcc/ada/libgnarl/s-tassta.adb +++ b/gcc/ada/libgnarl/s-tassta.adb @@ -1079,7 +1079,7 @@ package body System.Tasking.Stages is Stack_Guard (Self_ID, True); -- Initialize low-level TCB components, that cannot be initialized by - -- the creator. Enter_Task sets Self_ID.LL.Thread. + -- the creator. Enter_Task (Self_ID); diff --git a/gcc/ada/libgnarl/s-tporft.adb b/gcc/ada/libgnarl/s-tporft.adb index a7b4ce5e29a..66a9f02656e 100644 --- a/gcc/ada/libgnarl/s-tporft.adb +++ b/gcc/ada/libgnarl/s-tporft.adb @@ -98,6 +98,7 @@ begin System.Soft_Links.Create_TSD (Self_Id.Common.Compiler_Data, null, Sec_Stack_Size); + Self_Id.Common.LL.Thread := Thread; Enter_Task (Self_Id); return Self_Id; -- 2.43.2