From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117047 invoked by alias); 3 Apr 2017 16:57:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 117026 invoked by uid 89); 3 Apr 2017 16:57:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: sesbmg22.ericsson.net Received: from sesbmg22.ericsson.net (HELO sesbmg22.ericsson.net) (193.180.251.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Apr 2017 16:57:12 +0000 Received: from ESESSHC003.ericsson.se (Unknown_Domain [153.88.183.27]) by (Symantec Mail Security) with SMTP id FE.C5.26215.5EE72E85; Mon, 3 Apr 2017 18:57:11 +0200 (CEST) Received: from EUR01-DB5-obe.outbound.protection.outlook.com (153.88.183.145) by oa.msg.ericsson.com (153.88.183.27) with Microsoft SMTP Server (TLS) id 14.3.339.0; Mon, 3 Apr 2017 18:57:08 +0200 Authentication-Results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=ericsson.com; Received: from elxa4wqvvz1 (192.75.88.130) by HE1PR0701MB1883.eurprd07.prod.outlook.com (10.167.247.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1005.2; Mon, 3 Apr 2017 16:57:06 +0000 References: <20161129120702.9490-1-antoine.tremblay@ericsson.com> <2255ed6f-a146-026c-f871-00e9a33dfcf0@redhat.com> <86d1cy4umo.fsf@gmail.com> <86d1cxwgpk.fsf@gmail.com> <86h925ll3f.fsf@gmail.com> <8637dpldta.fsf@gmail.com> User-agent: mu4e 0.9.19; emacs 25.1.1 From: Antoine Tremblay To: Yao Qi CC: Antoine Tremblay , Pedro Alves , "gdb-patches@sourceware.org" Subject: Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping In-Reply-To: <8637dpldta.fsf@gmail.com> Date: Mon, 03 Apr 2017 16:57:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-ClientProxiedBy: BN6PR1001CA0002.namprd10.prod.outlook.com (10.174.84.15) To HE1PR0701MB1883.eurprd07.prod.outlook.com (10.167.247.23) X-MS-Office365-Filtering-Correlation-Id: 927f29af-d202-48c9-c53b-08d47ab27632 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(201703131423075)(201703031133081);SRVR:HE1PR0701MB1883; X-Microsoft-Exchange-Diagnostics: 1;HE1PR0701MB1883;3:51QOzyEHAvGoCpozlafv/zqgQZDfCwC1HLebPjYyFifg5y1HZwfReS7w3mDr4PBlNiV19xXvcNAmLEv5DVUSswhbILkLoSIo4qc91vRWS1p4659DsFkZuVLQ62ealBTOpIKn6ZuQ9Q6nNUNdhdYsfLRQ7buh3xXa3fEC806WtmNaJUYHiB6bWJHXuiW+4D0/k4m4PToE72H1+V1KMfZLEw8Kkvjd2dsEi6kRhSEi/kRqkQURTotTsKrggtEMLLIn6rV5VC+jSfrDUs6R76OhTa6KMJ5Slto/P+LUfRnT6FSZKmxMUzzFigRCsiy3PBhwqR1/GVSmHMFmp2X1TMPAEw==;25:HcpgsLlfkHEU7ENrtb2I1VyrKl/u2L1g5xwNuCEUr9NbXDzMHMtrXmgMGn5N0MwZhu1u79IJ2Kl9pYJZlYd91eXGk+hkAkB07aX3G9oHHk3NSfnWk4sW7ci3UdYKqPRTF0ihENCpnJAFXcwJeSZ8WgVBpgfizmNxuhPDnx3JJr3MZII1GxUBisDY5GTF0eYGvfe8arRqsbZy/4oJJHpho0lOf+c3Ch+8jWwDf5qNoHuxPq3xAf5OnP291zTXw4C4DVO62LiCQa3Lzzg1gw1QNC48DrD8hfkqkTaadXdvHD2Y70kmw5IBpKn+h3AeyjBOhwe3B5DSuIxtT5HFFUd5tzrXQ8jGp/Cw+MQHNlg8AABf1PHkhBwLGptOKUKJ27/8kdwtZ4BXggWKi+XBYifY9mWzRgcI5m6ddhf0tfPCq99GRtV8DwgIcqMBdSDM0/ePsZgAL8MAzbYIVzkXF27XCQ== X-Microsoft-Exchange-Diagnostics: 1;HE1PR0701MB1883;31:Wzowb45iCvXMN4Or5zowIuTMpVMu6n1ugQqEGDmoLHiekk2pHCdaFv8q9G6Fyn62SARBPhI5mzA+yLpcVoSCvNZKjwv581ZpTlCL8nADlP5VM5FkUKpINSM6zZT3nPMc8HSAgOof/BNY9MfXApvDPCTezLucE10EiSzQTl7sGIrO2o3u18MMbrQp8jMnIfvlnW3QPnjm4OFJ+rnQJgtTAT1JIG+MuyhPyGfOI/Qz+xo=;20:mDGKczaFZRb3SsjW/h8XUkvqW9iwv5zwT0lhHKp99TYKE7VOF/U1R1aPswhuc2aFF2ctN2dBMF+0T/tChSWI1U7LqqFkL9t8mOJv4UY6P6iWO2CVrhnmj/bXIEmsVYDJxp2ydZqwNsQ8siXdRb2+xFrDAwzwaIAI/PCWNa30+VaaHG+zUZ0zfTac3E3qf/PprKGNBw3iR+tOVbOuJV17wc2KBhI5nVWEG3Ya3jgriIcAAMmrLn1QzxjgKwOKZkUehSXWlRqDJLxdZyTMtmuopZr0wkF3OGTSsQVv71qCyXE2SfWdm00LZJpLp8TM8hEF6tCfO3Jbzef3L7D24yLe6ezvwnRQ1ABserL2ISx2NxMIy9E/ER19W4xXbaFolKyP9vDANkP7Bdm8DejEcVI/ywPPVBhJZKj3OOi8UsHapKzQJWiwtq2yl0Xjn+y6bBm+g4/kgvOPzZtbzslXOZfcEHfIR3izYOB7lNKoms8RzvludE8WpUYa9mAmEShKgHOt X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(37575265505322); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(8121501046)(5005006)(93006095)(93001095)(3002001)(10201501046)(6041248)(20161123564025)(20161123560025)(20161123555025)(201703131423075)(201702281528075)(201703061421075)(20161123562025)(6072148);SRVR:HE1PR0701MB1883;BCL:0;PCL:0;RULEID:;SRVR:HE1PR0701MB1883; X-Microsoft-Exchange-Diagnostics: 1;HE1PR0701MB1883;4:ZNn3aWeg5Z2InP/W3PScyhgrrIE95zkEaUuNE3OTcHhDhxCnsOoNkKDosYmwRbgk/6vLI1dulP63xJ1qB8zWRIZT7/5TZOiuidYkYNW9b6cDZTCzD4IwknYDZVD9a1yhQJyNfQ01SRclpSPdDRFuymqZuyehdjXdXNj7HHlFZKkM5ahvpg4GFgjkeK/dv5J07tunxXlJd/+Y5C6AUum/uBEg9DH2GcZ8V8fwVFXA3L/5oRwO6/+w1hn21SbqroH0NsbDihuxJo3nFwOqnxAxJ5Y9EdnAaF3ToVy5OOngeMhpty158XihOgoFlWeO7RzSXYPZ3+FQ0JJ52Fm4Z2RFpb8jNWmyD+YSaxFiTeaJGoWYiA70DgQ610hgE0TC/WiWzbru3KVAE0WAqJ0mIyqal5WeonWbpDQDmxKYqbOXiIcEjecfzU9UKsh5GSl7+bSGgzsBKnRF4pB8XCIGR3hOLwvr7YLVQ+w9Fjxj+omY0N/bT+tahZhqhJ5COUiiu3ClqSYVd3gx1F+jts3JjfBd0/Z+6dvFtEjl6tRMQBhMLBVSPRZ/18P8mVnEabqbXmAmnrJtzX829blblI17DOfCsAfdELW9NOIwsxFavWfjRlwGJUxmIjmuPMSuTt4m3va7VXUfkFY83lmHEXSDpGM5RT20KOdjImSgXSUqWodSmr1jW+5YkgYDvX14csyJvfNhkqqGJfBzL8m3oskdI6Hi6CxQchz2dZadbQlOqXEU5zz9iIraTDkGddgD5JypJkYxS753KLSHy9d2V9Z7xe2U3Q== X-Forefront-PRVS: 0266491E90 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(39400400002)(39850400002)(39860400002)(39410400002)(39840400002)(39450400003)(229853002)(5003940100001)(33646002)(53936002)(6486002)(86362001)(575784001)(25786009)(36756003)(6666003)(110136004)(38730400002)(2950100002)(3846002)(6116002)(4326008)(6916009)(5660300001)(50986999)(76176999)(54356999)(7736002)(305945005)(6246003)(6496005)(42186005)(48376002)(93886004)(2906002)(50466002)(54906002)(47776003)(81166006)(189998001)(66066001)(8676002);DIR:OUT;SFP:1101;SCL:1;SRVR:HE1PR0701MB1883;H:elxa4wqvvz1;FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;HE1PR0701MB1883;23:+GJ1v7p71KvYFxDmxHWXrqFmreFoH7U8mZP5oNK?= =?us-ascii?Q?KPbRsUf1772TW0fGByOl6qZXe93yicktGhSS9H5WQYl9MIqZxmM3m1eRwnNO?= =?us-ascii?Q?RoKhMbusJpYoCbuZwwZsCe7TJ1AlBuIgH14tekNx/QGsLyo1qiMfQN8qX5vj?= =?us-ascii?Q?G6TMcJ/UWwmdOHlqDSYgvfSpv+1bqd8cDaEiuP8AW0/p7zNgERgei7CjLMt6?= =?us-ascii?Q?7Gz2rJOD2Xv0Xqm4/9g0gR4eDHy5YjmKMUwnv0PEgjvuiujIZwpkWCyuc1lc?= =?us-ascii?Q?zPCiKiREum/5YlDckd+wvrDDxikX2ySgTMFI1wYKn4e/osTNPid43dz6UZxT?= =?us-ascii?Q?VQZ/yfpOtzNPumXqZsd0SuY9vxJ1zzzrFUXWSewRKuU5dcltR/my7NQqFgaM?= =?us-ascii?Q?Es6QZbqq47kK3REvGAwXDyD3vNFeO4Xwth4yf/eIphBa7+qv8ZCYw9tPZDTC?= =?us-ascii?Q?2PSRp8ZUrIDtEN0eSg3lPWjdhBK5PbIVQKOd5f4Zr2BsLFZMddZgGuj2wz5f?= =?us-ascii?Q?CeELeg9CMWUostd3CdoLAI3WxlbRFW6vMtwnb6hLJrHevhtv1wcax0wuVwDg?= =?us-ascii?Q?w52zx7KNuZCd+64x1e52cDgVGMs96S3GMI9StfYWAVObgFWS+EAyEydcp2rY?= =?us-ascii?Q?+paS6QUgaLIKxboF4vZPzzYBMz+Olr6s/3VsRcnHwY5MEUPUEvrIaTw3OuBK?= =?us-ascii?Q?KLofoY+33NePbCcrTsPmOtS8Rr8GHtamIY+QdGCUdKAQfFsqEdq1FJs+BLLQ?= =?us-ascii?Q?FkGjZ9RDbj3oa3kex9SucATt/XKQCg2kwh72SdmM/73IxOPo+675Xnq0Bt6o?= =?us-ascii?Q?rHFwxUjqyaFAJar4ibp3HPB9HXUmRyG2DatAOJplaVQ5PEUkyvlUC3dnJ93Z?= =?us-ascii?Q?mJd+riT2qDCfXQIyge22EtUxOfazZ+dOZH3w+tjKqkdNXpVfGyzjnDAjF7n/?= =?us-ascii?Q?0RPkaXKazoqeJpyHAmVwNyrtrVjefsKRokEbktzG1yj9oaKTOqenamH9B4eC?= =?us-ascii?Q?mVssx1AzCM8klOTiacuC/y9prqZ0fkHzOeTSjwJKlllWnw2T4jFYb0g6Eqag?= =?us-ascii?Q?GazDmmtTh6gFxtqCYParxYMbcOoP5MXidc2T6qTU0yEimFK7Hx769cFwXQW6?= =?us-ascii?Q?KqpEQ9qPElEg=3D?= X-Microsoft-Exchange-Diagnostics: 1;HE1PR0701MB1883;6:xGpZNy8GMakTTXoP/vhWrIc4Ggt3xV/h4YDLx3bX1hBt07LHSyuecVWHeXMFmtQfUoc0uvn5MfOWDqFEl1w71lKQqWGLY8FRkLbubTdWAcitQYi3Bx0ZN0Ev99qzD/hqVuDcf9ou8K+y4wlWi9IaBZwOAuzJMJHE0K2XWfixa7gq8Hl9aonMDsnhb276/lBDUydbW0F+YYK6VNWuBInxQdRmBP18x7gj0id+EP/jVWFHSw1m64NU61ou5ASsx5SjrJ0eOh7avBhTg6Yh1ltbmvyGnKOy9I4beOa5eNFazFqqB6ZsgE11N7pGaL+chxWLvIu2ReoDCuGj/1G7zrBSt9rTOfTMw48zBSSZLs2bK4AqEOdf/T+TISEG4V/n6230a87X5cD/tBJ3sVRMYPzBnA==;5:ldcMOGraJi0kDIYN61h1OGbZCS21SzdG26bUnrdGwoib3cHtKAvd3OSjQBxqXANZ775tZym7emyEmX4jc6dBwAekYtm1iOn9jPXDOrQJ2xM4NDE3E3EohqCImPhjRWpud7HmBjXPa59oShQUsuaYrA==;24:2bvwDrwkWqEObHXDWIURnziA58SIdOPjPYjJmR3SVtkpTkBuLE+n3whnvd57CKnxb8AWvkVtxIDRoXCWMwaaYa5GFs0LZ4BocGZ1mog/SeY= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;HE1PR0701MB1883;7:aC8tSqZkB7bYNflsRYV6jBpK3Egz11J91mOwVERbX8/tP0rGejtgcY3iWjtf9B/KChjXlDVo5xua8U6VywUgOSMYmAGj6qlD5sWVxUkfrODUr8vEALkaLR1Ip5f7VOQjo42BImSD902fSGAhGd9c1RD+OXmi/xrGib9pp9sy0RXvkdTNooZYsuUhrwyAfuq4aXj4IJ7eLsL0eaxqA9KW94x+I4qG/EHP7dq/p8vZ7qbR6SqRaGI8hFPWkVQdTjscn1es2J/6MfX/OZjdd2Qo9QQtUeztzaimnM8wAMkxp6Yrfc9ol7NYbKD0tPzSoBGg2xQ7+KBtKzgBe/v5zPsqcg== X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Apr 2017 16:57:06.3461 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0701MB1883 X-OriginatorOrg: ericsson.com X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00004.txt.bz2 Yao Qi writes: > Antoine Tremblay writes: > >> + if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0) >> + { >> + /* An IT instruction. Because this instruction does not >> + modify the flags, we can accurately predict the next >> + executed instruction. */ >> + itstate = inst1 & 0x00ff; >> + pc += thumb_insn_size (inst1); >> + >> + while (itstate != 0 && ! condition_true (itstate >> 4, status)) >> + { >> + inst1 = read_mem_uint (pc, 2,byte_order_for_code); >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + next_pcs.push_back (std::pair >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > > It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is > 16-bit. Good point this does not look correct indeed, a call to breakpoint_from_pc would be better at this point. > IMO, this function should only tell whether PC is in IT block > nor not. It shouldn't involve any breakpoint kinds selection. > Yes I think you're right, I could make it return std::pair And use breakpoint_from_pc if true , and BP_KIND_THUMB if false. >> + return next_pcs; >> + } >> + else if (itstate != 0) >> + { >> + /* We are in a conditional block. Check the condition. */ >> + if (! condition_true (itstate >> 4, status)) >> + { >> + /* Advance to the next executed instruction. */ >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + >> + while (itstate != 0 && ! condition_true (itstate >> 4, status)) >> + { >> + inst1 = read_mem_uint (pc, 2, byte_order_for_code); >> + >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + > > If all the following instructions' condition is false, breakpoint should > be set on the first instruction out side of IT block. We can still use > 16-bit thumb breakpoint. > >> + next_pcs.push_back (std::pair >> + (MAKE_THUMB_ADDR (pc), >> ARM_BP_KIND_THUMB2)); > > The same issue. > >> + return next_pcs; >> + } >> + else if ((itstate & 0x0f) == 0x08) >> + { >> + /* This is the last instruction of the conditional >> + block, and it is executed. We can handle it normally >> + because the following instruction is not conditional, >> + and we must handle it normally because it is >> + permitted to branch. Fall through. */ > > How do we fall through now? No breakpoints are added to the vector, so it falls through. > >> + } >> + else >> + { >> + int cond_negated; >> + >> + /* There are conditional instructions after this one. >> + If this instruction modifies the flags, then we can >> + not predict what the next executed instruction will >> + be. Fortunately, this instruction is archi2tecturally >> + forbidden to branch; we know it will fall through. >> + Start by skipping past it. */ >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + >> + /* Set a breakpoint on the following instruction. */ >> + gdb_assert ((itstate & 0x0f) != 0); >> + next_pcs.push_back (std::pair >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); >> + >> + cond_negated = (itstate >> 4) & 1; >> + >> + /* Skip all following instructions with the same >> + condition. If there is a later instruction in the IT >> + block with the opposite condition, set the other >> + breakpoint there. If not, then set a breakpoint on >> + the instruction after the IT block. */ >> + do >> + { >> + inst1 = read_mem_uint (pc, 2, byte_order_for_code); >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); >> + >> + if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) >> + { >> + next_pcs.push_back (std::pair >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); >> + } >> + else >> + { >> + next_pcs.push_back (std::pair >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB)); >> + } > > Why do you choose breakpoint in this way? > In the case of if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) we are in the IT block But if that is false we're after the IT block so it doesn't need a thumb2 breakpoint. (See the comment above in the code) >> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c >> index 6e6926a..f3845cf 100644 >> --- a/gdb/gdbserver/mem-break.c >> +++ b/gdb/gdbserver/mem-break.c >> @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, >> { >> int err_ignored; >> CORE_ADDR placed_address = where; >> - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); >> + int breakpoint_kind; >> + >> + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step >> + breakpoint. Get the kind of single-step breakpoint according to >> + the current register state. */ >> + if (type == single_step_breakpoint) >> + { >> + breakpoint_kind >> + = target_breakpoint_kind_from_current_state (&placed_address); > > I read my patch again, but it looks wrong. If we single-step an > instruction with a state change, like bx or blx, current get_next_pcs > correctly marked the address bit. However, with the change like this, > we'll get the wrong breakpoint kind. You're right, that's a problem however I think it could be fixed in arm_breakpoint_kind_from_current_state by adding a case where placed_address != current_pc in which case the thumb bit would be the deciding factor rather then arm_is_thumb_mode ()... I'll resubmit a proper patch with those fixes.