Message ID | 6c06a49b-8f4d-ad5f-53c1-984bd90a687b@mellanox.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 12324 invoked by alias); 25 Jan 2017 17:24:21 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 12284 invoked by uid 89); 25 Jan 2017 17:24:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=funky, modifier, forbid, offer X-HELO: EUR01-HE1-obe.outbound.protection.outlook.com Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=cmetcalf@mellanox.com; Subject: Re: [PATCH] tst-setcontext2: avoid bug from compiler optimization To: Torvald Riegel <triegel@redhat.com> References: <1484330513-61379-1-git-send-email-cmetcalf@mellanox.com> <8d532404-c012-c999-c60e-8d33a2932afb@mellanox.com> <1485343437.4311.156.camel@redhat.com> CC: <libc-alpha@sourceware.org> From: Chris Metcalf <cmetcalf@mellanox.com> Message-ID: <6c06a49b-8f4d-ad5f-53c1-984bd90a687b@mellanox.com> Date: Wed, 25 Jan 2017 12:23:41 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1485343437.4311.156.camel@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: DM5PR18CA0056.namprd18.prod.outlook.com (10.168.111.146) To DB6PR0501MB2760.eurprd05.prod.outlook.com (10.172.226.12) X-MS-Office365-Filtering-Correlation-Id: 2b942dd0-234b-4cb5-ef9e-08d44546efcb X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001); SRVR:DB6PR0501MB2760; X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2760; 3:C2ZYoOY8cb/mn01CRRBvwohoB27VvgOQaAKTO+hdh643SGK10VGUTZQZZi0bsgzpVi3Dc/71VLjxFeKSoFTfasIKKo/ssUMW6u1ytIQ2dbvyx6eH7q97NNjg0ZBi/W1n8UZR/4pgcyshyP+Q/KX5d7EAXizTIWHMnZ4qInQKdqf0nkdMx32P25YlHtr5BO62baVVJ/o4dFzF6qnB7OE4cHQuRkzXG8VyIQDXbEEVeAQBEezjfUOCYVNr35tzFKMaK+JN33Qi/8GzfLVblQJOJA==; 25:EiLxBaZVZpXIiVtVqudE8cbO8TY6tYVtsQHjnbiOQMbEhedBuE7rvPvkFA4F8GRIYe2AHTpC1X5GbrmFdoZnDrxOBAAh0wgM/KU9385Nf6bv8+kvC1eZcrywNb1az4n1Z6zyaoHtqoq5jjSBIyHKWPc29qiaO0Yomj0Y7K0WTV88c22rMcXTo2HCQo+SU6cEnbvhyJ+Beqgf7KSLtylKvcdZCqiKRZq4hTM82pfwhCT9f9w5yOpS4vYUtWHV7I65WcZbtKYHR6nIrX7ctn9DAxLzBYFIpu8eyigxU41ue42gMkxpvEZL9q7QNC/2HW+zUjwrNDYQxnQ9Ih24HckVDn+plSvJ7HdQd4Ntg6YmhzFtHkfVv3EfLI999w7BwyEJpdIp3dX+srV0WSlH+WcYbUvsijWzllA4EZyaPEDJmnEdjpEzeydF367/DChJJzoLaLax0Vg+MWvDIZ4uEgd+JQ== X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2760; 31:yMtTbLRQyfL4dMTLsN4XzPn3E15tQrjNWwUN2oelNkc7YR7CmbC2y+oEMRurjq5saNhuiBSMgoWEVW3im4yQ4XUHOm98Yw/PDZURuswDEtpFTtZ/X/x0Kmto8JhVxs0ra0aLe7Dvz1pPgz1oDxhTdUxdlYHypC0HpuD3hmxHb6gBuGosTSknMCwTixSowo8AM/rcdjsYgJ4kk85oXoifSrTzBgbgyfJUuz+tiSfKx9IkhjbCwaGcwmYJfABNgRahGTzyEeh+1muBvVHpwnUqPA==; 20:q8GahIL8p1s2TMMVLgih+gP7K9fKMRah1JWFstzc4DWPEi+DxrtEZqIJ/sRQgWsxutW5dhEgxplWFBZmhawS8/8x46Z2oy5NpenGrM/pWGOBhIcVmZjFrM9PP0O4rJHb6Ln3R4cdVV5whWkogBZ/hx0maOJlNxHIEVqm/+uW1vSOghTWf3HxGz4oAEgC1HfrJYvMz58z3LMCklBGR6a4K/OwpikP/K3ZCKCM7VnADNZ89SM31Jid9Hnh8P2E3Cg7umpQtELOpWeReKgC/RY1GWKjk1oIuh7yVE6EC2xOt2ifKKmwNzmlRr4GCNcPxmaWdL3egW20XcIjsc/FFMYKtoS7UI6Xtnq2aziu6QZwHyKeNpndmK2RGy7CEgxs2zfcdGdRM54YrSzqXotbR9eVQE05eTRuqxg3DiFn0BDKESMq+NqYt1psMzeJ4BMKqFysz8Ef58KCV8X0t5FTi9DPz3+jdX7Z3+ggAUuDG0+HlDHRUFi3YxfzznXijkWilGD7 X-Microsoft-Antispam-PRVS: <DB6PR0501MB2760515E6381177576A00F1FB2740@DB6PR0501MB2760.eurprd05.prod.outlook.com> X-Exchange-Antispam-Report-Test: UriScan:(190756311086443)(171992500451332); X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(5005006)(8121501046)(10201501046)(3002001)(6055026)(6041248)(20161123560025)(20161123562025)(20161123555025)(20161123564025)(6072148); SRVR:DB6PR0501MB2760; BCL:0; PCL:0; RULEID:; SRVR:DB6PR0501MB2760; X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2760; 4:GArE4mq+KAmyoBA7C5JrGuu0XkwF11SrEaYlegNOzBaqAJM1dfFg5msvqzP6h86MXQd4clhtN5A+rXmhogfZ/G7BtQHx8Jv9n5gVrsPM9c86oc0tivDgzAFAScK8QvQVnttfF+7b3VQVxkfGggElQzfPtPkCC4j9NT158jmRxTqwWx8m8HVsnggVccFr8M6ZpD7yPLYFHkWcHLjnjPwKneyu15ZELr8064vy9ILfE6QmeZiJgqpSKVSLMbIkTF5jfkUvBCJXCJXwc/QXhIASXIanXsCjJY39/+Y7hTbGRT1TerlOsq1T6gT9bPHy0JsEDlHhy1jIDjltuzzpePhsHHvrj9tLgP/rdhdX+fcWdtIwyu/Kc9tkmfVIoR9sECqqKDQXwVU1VBCnJ24K3hc9oOw6nch7o8W5o6wVp/YvGi8ehM24jsnjlpBpVCr8ghYbAtEkAJYdIs1g7oYxf3slkpuO2Iey2Q1CzCXGnDmGExF9f0AJqFdkXy9lTxWxngSbP1DizKGKyvKnlckrbZsliDRwSl6RDMBs5/XNDQBzO1fzkvezNxlSGXKzTJarPNpVYllAY6fuArXLRetdVmmvKvviO/E9G/xR5DeJ5e2iApxYsklK6Mt6K/YRCGdXpyKO1niWicfvdmrtKD4o8cUllnz8E1jbP3EAo4JcoE+x4V4= X-Forefront-PRVS: 01986AE76B X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(4630300001)(6009001)(6049001)(7916002)(39410400002)(39850400002)(39860400002)(39840400002)(39450400003)(377454003)(189002)(199003)(24454002)(377424004)(31686004)(81166006)(53936002)(42186005)(47776003)(23676002)(68736007)(3846002)(4326007)(106356001)(6116002)(2906002)(50466002)(36756003)(86362001)(8676002)(92566002)(64126003)(81156014)(105586002)(31696002)(83506001)(65956001)(65806001)(76176999)(66066001)(54356999)(50986999)(305945005)(25786008)(97736004)(6916009)(33646002)(101416001)(230700001)(5660300001)(110136003)(65826007)(6486002)(229853002)(4001350100001)(6306002)(38730400001)(7736002)(6666003)(90366009)(189998001)(2950100002)(77096006)(18886065003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB6PR0501MB2760; H:[10.15.7.185]; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; Received-SPF: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtEQjZQUjA1MDFNQjI3NjA7MjM6N1VqODE4bFNUbzBnQnJ0THRFajhhR3Nu?= =?utf-8?B?SjE0OUhqT0xldlZwWmJLSVp6UWw2MzVVTmMyUFp6dzF0ZGNYdzFuamZFcEl6?= =?utf-8?B?NC9UQzJBdk5kSnNPVU1jYytRUlVMSmIrc1J3bm9PNmFmdGZnNnVQWEF4ZjNv?= =?utf-8?B?Q0g0bFFzS3o4Nk1QTFV5UHp4d3oyV2J3bytxQy9CQWtMaGJwL1hNb2V0QVd2?= =?utf-8?B?RzZGSkhGeDB3cHphWkY2bDN3L3VEd0JDYmpTZmM0N0x3UzIraFR6bGVmcWQy?= =?utf-8?B?elpIYzRYUmlSQ2xSblJJekJ5dmRkY0RGdy9hcFZrWHFFQUhaZHhYc3BxaGJj?= =?utf-8?B?YjdMakVxWnpGOFQ4bFhuUFFXbkR2dXh0dXBVQS9JVUR1endUcnVUODlwbHpY?= =?utf-8?B?dDJoeTBHREU2YXV2dWk0WnNQeFJFcXdOZ2psWmNMNStvODFkS2pyTFZOMHhW?= =?utf-8?B?R2VzRE9kc2EvbU5RVXpDMVhpZ1VEYjczRDZadzl3UmFmcVZwRjNjcWVkMXB4?= =?utf-8?B?ZXpsSTlMWUcrQTlLQmgrK3ZSelIySERSVnBQSjQrcFBycnU3S3BBaWhUMnNM?= =?utf-8?B?aFFpbWVCTlpuQkc4K085UkNpZWZIUUE0RnQwU3NTWXNNbytZeklzL3lwZHNt?= =?utf-8?B?Y0xmc0F1MHFYWE1ScXRTc1liY0FaZllIN3FibDBrdkVJaEJvVGoyek4vd1dx?= =?utf-8?B?RnVZdmNHYmZTMzJJNURpV3h4NDB4cDZVbVlCb1V3QnB3bzF0RUNra1BzZVdx?= =?utf-8?B?TWpveXZYSDVVclRnbUU3aXQybkRjakdFZEJ2VzFhbTZ3TDgrUE9BaFJNUnp6?= =?utf-8?B?MHQ1eExwWG8ydjBmcW5KeXNOUHJHdFRHMXl4dUpKc0ZTLzBwNitBUHQ2WXFw?= =?utf-8?B?TXVzMjYvai9ySkJvQ1daTlNHd2hNa2xCYkxqekRxWlFQY0U5Y2tuSlN5a2Ew?= =?utf-8?B?NE92TUJIZE0zdzdIZ1QvVnhiRDNCMXZQeVFtb0pyYlNucnFGeXJ1SjJEdnI3?= =?utf-8?B?cjRzN2xOeXl3bXo0eDluZzhjZmVtaHprVTcwT3BjQkcxaDJlREVIQnVIQlF2?= =?utf-8?B?SXpNVzlwSkVZZ0E0KzBienpBbzBsY0ZlZ2FsYmhpcTVCWGpTeFlYeUVOS1JV?= =?utf-8?B?L3lMYStxR3oxT3hhSW1zUklyeklpdnpoUU1lcWxXakxxTUwxaUVVaTlqR1l5?= =?utf-8?B?WWxqS3VTYk5IUmM1TURHMW9ZMmZ6Y3VNL1NTOFBablI4Vkl6K1BaYmoydTRK?= =?utf-8?B?OU0vUnpTYmtVbHNLY1hzR3N2MEw2ZTZYS2Vqa0tYcDd2NldGRCtQTXhGYVhU?= =?utf-8?B?SXRibmlwMUxScE54ZnhGSm1WUzhJWS9uK0JBVVdOaUlRaHBvY2t3OUdtQVJU?= =?utf-8?B?RXVVTzhNK0VFRjNGTjNPdlJDV1JSTVVYTEVTNDRWQzJmTzZGOS9WZm85ay9S?= =?utf-8?B?K2JxWUxhQWU0VzRETEE2clRIMWN6RTZXZmJITlNCVGluaUZ3Rll3aWFTaks5?= =?utf-8?B?OEF5U0NkbDVrRjJ0QWN0c0dkbVo0R3Rta1BVVS9yV1hJOC9XQ0pDMndVSk1W?= =?utf-8?B?aHh5a21GK3dMZU8ySWtOWjdGYVhER1FoeWJEQnQzMTc5UmR3Nzdpd0I0cWxE?= =?utf-8?B?Vm1ZQm1Zc0UwSy9VSkovUmpaQUQ3S3F1dW56bDR6ejNCSzVYaGVSM2V2dnF4?= =?utf-8?B?b1JzdWVwRWhxTW1uWmxaMjZ2ZkQ0MlRJYlpRclJ5ZEphUUVMRmhJT09yZm9w?= =?utf-8?B?V01HZW9xcUYrUHlCc3FBUnBJc0tPUHFBUm9lOTEremdRVlJKMFdpeGxjcTNz?= =?utf-8?B?REIwZy9WZTAvOVYxN24ycVRSTEY3aWJjM2phajZmTjlCUnpic1RTcSsvZFZk?= =?utf-8?B?QWUxSmE0SFAzby9oWFd5aHRKbXVDd21uVkdOb1JndC92b3lQRkJKajduT2Iz?= =?utf-8?B?dE9hOTI4bVZNd0RqUzB2RnFvckpEYkIwTUpob00vcFY2ODRxS1NlVTcrZ0dK?= =?utf-8?B?eU9aTVlXQ1NBUkxuYUtPSURCc1VKZEVYZ0FpdlJ3OUZyTlRYNHZGbk4rWmw1?= =?utf-8?B?MldRbVplcU9RYWpGY3BzOWFHZ0xoT2ZWalBQa20rVnFvelM4WkFQaEI3cWIw?= =?utf-8?B?SEJtZz09?= X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2760; 6:fQtm4B0s5z2c/sBuveSrqsGESkB1S93db+RVXrST8J+//lxONG5ahaBPsECDwsA5+dTsYSpMPhLJzKmoVzinkU71kYZ1r6SQpNed4gU0hZSCPhUFrf0Pc5JDkxAWn3uI8s03YU5e/ImgyPNEuNaYPf9xiRJiH3O4+tBCIGG4EgcIhNc/7OIaKGFyu6EaljOlfs6jGIIkM2NTIhqTTWp1KTo2idUjnW6vtS8X+7JulK1sMW1tnNfHVesIAM2rjvNj+zEx+F+tEJ31HMnqKkjIM3K+6BLcXHQHmAgQmRo6iF/kt2E2dXjcR+pOlj5KeDDuHqw3kpsTdtM80EAtJYeVQZekkXNNxeAo+B2HESguYwvgrNQmSUtAMPi7z28QWM7HFgOz3z9RmzyHG+vYLeYaONB/bUmbFSX8FeWn62c2OnoW/msm2mL/uC0ahwUNGJyLad3MglIM4GAlewwgsfWeIA==; 5:yd0dttM34v0tPXja0UbiomYIJO6UbabGtzEyxGnUvtw0wAnVD/10Ue6fdn4/hiVGplDkEtXDKmuughkEukOu6RiHRaukmxFTk4u5E9ndEk1ihZvaRkVLFCeyZBKj2VlxqapKvOD1VrmJsgOEdhftu3EIWHqSJKk3A30XNpnEzKg=; 24:deUvj7yXAim9yQpGe/7i8dKzhBxIoNgTrABoT/garFpAapxgcPwNBQTw25S078DmdnM+D8++AiW7qHqZJWHg4uyS38xeJ8IL+04bsS57AkA= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; DB6PR0501MB2760; 7:9PWzteQYxYqRgJpsRcdILE/acrGL3AlmUJpBGPqvA+ayIGufeW86zo3SmQDJy1GfyZqzLkLljzDEZ18A0keS9pnEG7XXMZPaUbBSnkvFM0doniAIvWe1cASD9ReszxhpgoasMmMyI75yQ/yAmm9qec7enMB9YsOhVp6YPdsPeLEXXXRJEAuaW8fIfQk9nrBRlG86JT/oyBMt+oErFyDVbmZMO1nSs3GDio6SoKqfT9e2bhTnqGlIGGBfSY3RUxivyrPOzBN0B209BP9qs/8J+iWnaunRomNQFaxqtwwikGxhEss6kDlEA+EvUNBUabXcPPESP1gcog5lbXxjbPZP8IhXSbW8jN9r+j7KrM67I3F0W521p3s9N4mxMUrYY36dbMqVJbW+MTRWbl2SRR2rgcwDwxTRdB1hhgQFUUKh1cA0N/3QVGI3qv8bzXU5PInjc9vLqp+Lv8vhQ+rE/nx8MQ== X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2017 17:23:53.2133 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0501MB2760 |
Commit Message
Chris Metcalf
Jan. 25, 2017, 5:23 p.m. UTC
On 1/25/2017 6:23 AM, Torvald Riegel wrote: > On Tue, 2017-01-24 at 19:35 -0500, Chris Metcalf wrote: >> Ping! I will plan to commit this later this week if no one objects; it seems >> like a straightforward bug avoidance. >> >> On 1/13/2017 1:01 PM, Chris Metcalf wrote: >>> With an uninitialized oldctx, the compiler is free to observe that >>> the only path that sets up a value in oldctx is through the >>> "if (global == 2)" arm, in which arm we apparently return 0 without >>> referencing oldctx again. >>> >>> Then, after the "if" cascade, the compiler can inline the "check" >>> function and then observe that the sigset_t "set" variable there >>> is only used locally, before any apparent uses of oldctx, and as a >>> result it can decide to use the same stack region for both variables. >>> Unfortunately this has the effect of clobbering oldctx when we call >>> sigprocmask, and results in the test failing. >>> >>> By initializing oldctx at the top, we let the compiler know that it >>> has a value that has to be preserved down to the part of the code >>> after the "if" cascade, and it won't try to place another variable >>> in that same part of the stack. > The compiler would also know what the initial value is, which it could > store somewhere else, which then would still allow for reuse of a stack > slot. Good point. > I agree with Florian that the compiler needs to be made aware that > getcontext can return twice, or something to that effect. This would > tell it that it has to reason about the lifetimes of variables > differently. The problem is that "returns_twice" doesn't offer the semantics we want. It ensures that register-allocated variables are handled properly, i.e. everything is saved to the stack frame prior to calling the function. But here the issue is that the stack frame itself isn't being set up in a way that actually works. And in practice, tagging getcontext and swapcontext with attribute((returns_twice)) does not fix the bug. (It does seem like doing so isn't a bad idea, but it is beyond the scope of fixing this one test bug.) Another way to fix the problem is to make the context variables function static, which should forbid the compiler from doing anything funky with them. (Although do_test itself is static, it is called from main, and the compiler has to assume main could get called again and expect to find the updated context variables still updated, so it can't trickily ignore the static modifier or anything like that, I think.) commit 5a054bf335e350e96e2a38b5d2573f4f26a2185a Author: Chris Metcalf <cmetcalf@mellanox.com> Date: Fri Jan 13 12:50:50 2017 -0500 tst-setcontext2: avoid bug from compiler optimization With an uninitialized oldctx, the compiler is free to observe that the only path that sets up a value in oldctx is through the "if (global == 2)" arm, in which arm we apparently return 0 without referencing oldctx again. Then, after the "if" cascade, the compiler can inline the "check" function and then observe that the sigset_t "set" variable there is only used locally, before any apparent uses of oldctx, and as a result it can decide to use the same stack region for both variables. Unfortunately this has the effect of clobbering oldctx when we call sigprocmask, and results in the test failing. By making oldctx (and ctx) have static scope, we forbid the compiler from performing this optimization. The compiler will be required to allocate them to separate memory so that they would have their updated values if the do_test function were to be invoked a second time. (We know that doesn't happen, but the compiler can't prove it solely by examination of this compilation unit.) Seen on tilegx with gcc 4.8 at -O3.
Comments
On Jan 25 2017, Chris Metcalf <cmetcalf@mellanox.com> wrote:
> By making oldctx (and ctx) have static scope, we forbid the compiler
There's no such thing as static scope, what you are meaning is the
storage duration.
Andreas.
On Wed, 2017-01-25 at 12:23 -0500, Chris Metcalf wrote: > On 1/25/2017 6:23 AM, Torvald Riegel wrote: > > On Tue, 2017-01-24 at 19:35 -0500, Chris Metcalf wrote: > >> Ping! I will plan to commit this later this week if no one objects; it seems > >> like a straightforward bug avoidance. > >> > >> On 1/13/2017 1:01 PM, Chris Metcalf wrote: > >>> With an uninitialized oldctx, the compiler is free to observe that > >>> the only path that sets up a value in oldctx is through the > >>> "if (global == 2)" arm, in which arm we apparently return 0 without > >>> referencing oldctx again. > >>> > >>> Then, after the "if" cascade, the compiler can inline the "check" > >>> function and then observe that the sigset_t "set" variable there > >>> is only used locally, before any apparent uses of oldctx, and as a > >>> result it can decide to use the same stack region for both variables. > >>> Unfortunately this has the effect of clobbering oldctx when we call > >>> sigprocmask, and results in the test failing. > >>> > >>> By initializing oldctx at the top, we let the compiler know that it > >>> has a value that has to be preserved down to the part of the code > >>> after the "if" cascade, and it won't try to place another variable > >>> in that same part of the stack. > > The compiler would also know what the initial value is, which it could > > store somewhere else, which then would still allow for reuse of a stack > > slot. > > Good point. > > > I agree with Florian that the compiler needs to be made aware that > > getcontext can return twice, or something to that effect. This would > > tell it that it has to reason about the lifetimes of variables > > differently. > > The problem is that "returns_twice" doesn't offer the semantics we want. There are similarities to setjmp/longjmp, I think. From C11 7.13.2.1: All accessible objects have values, and all other components of the abstract machine have state, as of the time the longjmp function was called, except that the values of objects of automatic storage duration that are local to the function containing the invocation of the corresponding setjmp macro that do not have volatile-qualified type and have been changed between the setjmp invocation and longjmp call are indeterminate. oldctx is modified between the getcontext (like setjmp) and effective longjmp part of swapcontext. > It ensures that register-allocated variables are handled properly, i.e. > everything is saved to the stack frame prior to calling the function. But > here the issue is that the stack frame itself isn't being set up in a way that > actually works. And in practice, tagging getcontext and swapcontext with > attribute((returns_twice)) does not fix the bug. (It does seem like doing so > isn't a bad idea, but it is beyond the scope of fixing this one test bug.) > > Another way to fix the problem is to make the context variables function static, > which should forbid the compiler from doing anything funky with them. > (Although do_test itself is static, it is called from main, and the compiler > has to assume main could get called again and expect to find the updated > context variables still updated, so it can't trickily ignore the static modifier > or anything like that, I think.) I think that we need the returns_twice attribute, but we also shouldn't put oldctx on the stack (unless it's marked volatile).
On 1/25/2017 12:54 PM, Andreas Schwab wrote: > On Jan 25 2017, Chris Metcalf <cmetcalf@mellanox.com> wrote: > >> By making oldctx (and ctx) have static scope, we forbid the compiler > There's no such thing as static scope, what you are meaning is the > storage duration. Thanks, fixed in my tree. I know the difference but apparently my brain was an autopilot when I was typing. :-)
On 1/25/2017 1:05 PM, Torvald Riegel wrote: > On Wed, 2017-01-25 at 12:23 -0500, Chris Metcalf wrote: >> On 1/25/2017 6:23 AM, Torvald Riegel wrote: >>> I agree with Florian that the compiler needs to be made aware that >>> getcontext can return twice, or something to that effect. This would >>> tell it that it has to reason about the lifetimes of variables >>> differently. >> The problem is that "returns_twice" doesn't offer the semantics we want. > There are similarities to setjmp/longjmp, I think. > > From C11 7.13.2.1: > All accessible objects have values, and all other components of the > abstract machine have state, as of the time the longjmp function was > called, except that the values of objects of automatic storage duration > that are local to the function containing the invocation of the > corresponding setjmp macro that do not have volatile-qualified type > and have been changed between the setjmp invocation and longjmp call are > indeterminate. > > oldctx is modified between the getcontext (like setjmp) and effective > longjmp part of swapcontext. Yes, there are certainly similarities. Note that swapcontext also acts like getcontext as a returns_twice function. >> It ensures that register-allocated variables are handled properly, i.e. >> everything is saved to the stack frame prior to calling the function. But >> here the issue is that the stack frame itself isn't being set up in a way that >> actually works. And in practice, tagging getcontext and swapcontext with >> attribute((returns_twice)) does not fix the bug. (It does seem like doing so >> isn't a bad idea, but it is beyond the scope of fixing this one test bug.) >> >> Another way to fix the problem is to make the context variables function static, >> which should forbid the compiler from doing anything funky with them. >> (Although do_test itself is static, it is called from main, and the compiler >> has to assume main could get called again and expect to find the updated >> context variables still updated, so it can't trickily ignore the static modifier >> or anything like that, I think.) > I think that we need the returns_twice attribute, but we also shouldn't > put oldctx on the stack (unless it's marked volatile). I'm not sure we need or want the returns_twice attribute, given that gcc already has special code for recognizing setjmp, vfork, etc. Currently glibc does not use the returns_twice attribute anywhere in its source tree. If getcontext or swapcontext is missing from gcc (and I don't know that it is) then presumably it's a gcc bug.
On Wed, 25 Jan 2017, Chris Metcalf wrote: > I'm not sure we need or want the returns_twice attribute, given that > gcc already has special code for recognizing setjmp, vfork, etc. Currently > glibc does not use the returns_twice attribute anywhere in its source tree. > If getcontext or swapcontext is missing from gcc (and I don't know that > it is) then presumably it's a gcc bug. Recognizing based on function names and setting ECF_* is definitely an obsolescent approach in GCC, compared to the approach of GCC defining built-in functions using the appropriate attributes (and defining built-in functions is problematic for some of these functions because of the header-defined types involved in the function prototypes). Defining built-in functions has the limitation of not providing the attribute with -fno-builtin. Where an attribute is needed for correctness, as here, it should be included in the glibc headers. GCC could then in future eliminate the special_function_p special-casing, replacing it either by built-in functions or by making fixincludes add the attribute to headers that don't already have it. (Should clone have this attribute, just like vfork? If so, that's a case missing from GCC.)
I'd like to commit the one-line patch below that just declares the context variables for test-setcontext2 as static. I think the consensus is that this is necessary, and orthogonal to the question of whether and how the returns_twice attribute is associated with various functions in glibc; it fixes a test failure for tilegx. (The patch is quoted below; I've updated the commit message to avoid the think-o observed by Andreas.) I'll do this early next week unless someone raises an objection. On 1/25/2017 12:23 PM, Chris Metcalf wrote: > On 1/25/2017 6:23 AM, Torvald Riegel wrote: >> I agree with Florian that the compiler needs to be made aware that >> getcontext can return twice, or something to that effect. This would >> tell it that it has to reason about the lifetimes of variables >> differently. > > The problem is that "returns_twice" doesn't offer the semantics we want. > It ensures that register-allocated variables are handled properly, i.e. > everything is saved to the stack frame prior to calling the function. But > here the issue is that the stack frame itself isn't being set up in a way that > actually works. And in practice, tagging getcontext and swapcontext with > attribute((returns_twice)) does not fix the bug. (It does seem like doing so > isn't a bad idea, but it is beyond the scope of fixing this one test bug.) > > Another way to fix the problem is to make the context variables function static, > which should forbid the compiler from doing anything funky with them. > (Although do_test itself is static, it is called from main, and the compiler > has to assume main could get called again and expect to find the updated > context variables still updated, so it can't trickily ignore the static modifier > or anything like that, I think.) > > commit 5a054bf335e350e96e2a38b5d2573f4f26a2185a > Author: Chris Metcalf <cmetcalf@mellanox.com> > Date: Fri Jan 13 12:50:50 2017 -0500 > > tst-setcontext2: avoid bug from compiler optimization > > With an uninitialized oldctx, the compiler is free to observe that > the only path that sets up a value in oldctx is through the > "if (global == 2)" arm, in which arm we apparently return 0 without > referencing oldctx again. > > Then, after the "if" cascade, the compiler can inline the "check" > function and then observe that the sigset_t "set" variable there > is only used locally, before any apparent uses of oldctx, and as a > result it can decide to use the same stack region for both variables. > Unfortunately this has the effect of clobbering oldctx when we call > sigprocmask, and results in the test failing. > > By making oldctx (and ctx) have static scope, we forbid the compiler > from performing this optimization. The compiler will be required > to allocate them to separate memory so that they would have their > updated values if the do_test function were to be invoked a second > time. (We know that doesn't happen, but the compiler can't prove > it solely by examination of this compilation unit.) > > Seen on tilegx with gcc 4.8 at -O3. > > diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c > index 07fb974c4684..c937f6c396db 100644 > --- a/stdlib/tst-setcontext2.c > +++ b/stdlib/tst-setcontext2.c > @@ -87,7 +87,7 @@ handler (int __attribute__ ((unused)) signum) > static int > do_test (void) > { > - ucontext_t ctx, oldctx; > + static ucontext_t ctx, oldctx; > struct sigaction action; > pid_t pid; > >
On 01/27/2017 09:34 PM, Chris Metcalf wrote: > I'd like to commit the one-line patch below that just declares the context > variables for test-setcontext2 as static. I think the consensus is that > this is necessary, No, I think the test is supposed to work as-is; it is valid. So if this test is broken, it is a bug in the implementation of the context switching functions (as they are visible to applications, i.e. the fix might not be in the functions themselves, but adjusting their calling conventions, inhibiting GCC optimizations which make incorrect assumptions, etc.). If we do not have a fix right now, it is better to let the test keep failing (perhaps XFAIL it), rather than tweaking it so that it tests something else. Thanks, Florian
On 1/27/2017 4:30 PM, Florian Weimer wrote: > On 01/27/2017 09:34 PM, Chris Metcalf wrote: >> I'd like to commit the one-line patch below that just declares the context >> variables for test-setcontext2 as static. I think the consensus is that >> this is necessary, > > No, I think the test is supposed to work as-is; it is valid. > > So if this test is broken, it is a bug in the implementation of the context switching functions (as they are visible to applications, i.e. the fix might not be in the functions themselves, but adjusting their calling conventions, inhibiting GCC optimizations which make incorrect assumptions, etc.). > > If we do not have a fix right now, it is better to let the test keep failing (perhaps XFAIL it), rather than tweaking it so that it tests something else. We would be waiting on functionality from gcc that doesn't currently exist, correct? The current returns_twice semantics is insufficient to describe the problem that we currently see with shared stack frames. It's possible that returns_twice could be extended to also have the semantics of "don't share my arguments' stack space with any other variables in this function" pretty reasonably, but I don't want to try to guess what will make most sense to the gcc folks. I can certainly let the test keep failing and just open a bug to reference it and capture some of this discussion.
On 01/27/2017 10:53 PM, Chris Metcalf wrote: > On 1/27/2017 4:30 PM, Florian Weimer wrote: >> On 01/27/2017 09:34 PM, Chris Metcalf wrote: >>> I'd like to commit the one-line patch below that just declares the >>> context >>> variables for test-setcontext2 as static. I think the consensus is that >>> this is necessary, >> >> No, I think the test is supposed to work as-is; it is valid. >> >> So if this test is broken, it is a bug in the implementation of the >> context switching functions (as they are visible to applications, i.e. >> the fix might not be in the functions themselves, but adjusting their >> calling conventions, inhibiting GCC optimizations which make incorrect >> assumptions, etc.). >> >> If we do not have a fix right now, it is better to let the test keep >> failing (perhaps XFAIL it), rather than tweaking it so that it tests >> something else. > > We would be waiting on functionality from gcc that doesn't currently > exist, correct? The current returns_twice semantics is insufficient to > describe the problem that we currently see with shared stack frames. I think this could still be a GCC regression in the handling of the returns_twice attribute, introduced when GCC started sharing stack slots more aggressively. When returns_twice was initially implemented and documented, the question of stack slot reuse did not arise because GCC probably did not do it at all. > It's possible that returns_twice could be extended to also have the > semantics of "don't share my arguments' stack space with any other > variables in this function" pretty reasonably, but I don't want to try to > guess what will make most sense to the gcc folks. GCC already has this behavior on x86_64. This could be an accident. > I can certainly let the test keep failing and just open a bug to reference > it and capture some of this discussion. Yes, would you please file a GCC bug with a preprocessed input file? Thanks, Florian
On Mon, 2017-01-30 at 12:57 +0100, Florian Weimer wrote: > On 01/27/2017 10:53 PM, Chris Metcalf wrote: > > On 1/27/2017 4:30 PM, Florian Weimer wrote: > >> On 01/27/2017 09:34 PM, Chris Metcalf wrote: > >>> I'd like to commit the one-line patch below that just declares the > >>> context > >>> variables for test-setcontext2 as static. I think the consensus is that > >>> this is necessary, > >> > >> No, I think the test is supposed to work as-is; it is valid. > >> > >> So if this test is broken, it is a bug in the implementation of the > >> context switching functions (as they are visible to applications, i.e. > >> the fix might not be in the functions themselves, but adjusting their > >> calling conventions, inhibiting GCC optimizations which make incorrect > >> assumptions, etc.). > >> > >> If we do not have a fix right now, it is better to let the test keep > >> failing (perhaps XFAIL it), rather than tweaking it so that it tests > >> something else. > > > > We would be waiting on functionality from gcc that doesn't currently > > exist, correct? The current returns_twice semantics is insufficient to > > describe the problem that we currently see with shared stack frames. > > I think this could still be a GCC regression in the handling of the > returns_twice attribute, introduced when GCC started sharing stack slots > more aggressively. When returns_twice was initially implemented and > documented, the question of stack slot reuse did not arise because GCC > probably did not do it at all. I may be misremembering, but I think I had to add a returns_twice attribute to the function call that starts a transaction (for GCC's support for transactional memory). This was motivated by seeing sharing of stack slots, and adding the returns_twice was supposed to fix this back then. Which is to say that this sounds like a regression on GCC's side to me, too.
On 1/30/2017 6:57 AM, Florian Weimer wrote: > On 01/27/2017 10:53 PM, Chris Metcalf wrote: >> On 1/27/2017 4:30 PM, Florian Weimer wrote: >>> On 01/27/2017 09:34 PM, Chris Metcalf wrote: >>>> I'd like to commit the one-line patch below that just declares the >>>> context >>>> variables for test-setcontext2 as static. I think the consensus is that >>>> this is necessary, >>> >>> No, I think the test is supposed to work as-is; it is valid. >>> >>> So if this test is broken, it is a bug in the implementation of the >>> context switching functions (as they are visible to applications, i.e. >>> the fix might not be in the functions themselves, but adjusting their >>> calling conventions, inhibiting GCC optimizations which make incorrect >>> assumptions, etc.). >>> >>> If we do not have a fix right now, it is better to let the test keep >>> failing (perhaps XFAIL it), rather than tweaking it so that it tests >>> something else. >> >> We would be waiting on functionality from gcc that doesn't currently >> exist, correct? The current returns_twice semantics is insufficient to >> describe the problem that we currently see with shared stack frames. > > I think this could still be a GCC regression in the handling of the returns_twice attribute, introduced when GCC started sharing stack slots more aggressively. When returns_twice was initially implemented and documented, the question of stack slot reuse did not arise because GCC probably did not do it at all. > >> It's possible that returns_twice could be extended to also have the >> semantics of "don't share my arguments' stack space with any other >> variables in this function" pretty reasonably, but I don't want to try to >> guess what will make most sense to the gcc folks. > > GCC already has this behavior on x86_64. This could be an accident. > >> I can certainly let the test keep failing and just open a bug to reference >> it and capture some of this discussion. > > Yes, would you please file a GCC bug with a preprocessed input file? OK, so upgrading to a newer gcc seems to have fixed the problem for me. I was using 4.8.2, which has the problem, as does 4.8.5 (the current 4.8 release). However, I don't see it in 5.4, 6.3, or a gcc tip build from earlier this month, so I think I will just upgrade my gcc to 6.3 and not worry about it. I'm not sure how aggressively the gcc folks backport these kinds of things to earlier releases; if it seems like it would be helpful, I suppose I could file a bug against 4.8 on tilegx specifically?
On 01/31/2017 05:04 PM, Chris Metcalf wrote: > I'm not sure how aggressively the gcc folks backport these kinds of > things to earlier releases; if it seems like it would be helpful, I > suppose I could file a bug against 4.8 on tilegx specifically? The 4.8 and 4.9 branches are closed upstream. It looks like all active branches have already been fixed, so no further action is needed. Thanks, Florian
On Jan 31 2017, Chris Metcalf <cmetcalf@mellanox.com> wrote: > I'm not sure how aggressively the gcc folks backport these kinds of > things to earlier releases; if it seems like it would be helpful, I > suppose I could file a bug against 4.8 on tilegx specifically? GCC 4.8 is already out of support. Andreas.
diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c index 07fb974c4684..c937f6c396db 100644 --- a/stdlib/tst-setcontext2.c +++ b/stdlib/tst-setcontext2.c @@ -87,7 +87,7 @@ handler (int __attribute__ ((unused)) signum) static int do_test (void) { - ucontext_t ctx, oldctx; + static ucontext_t ctx, oldctx; struct sigaction action; pid_t pid;