From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 80178 invoked by alias); 11 Dec 2015 15:06:11 -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 80162 invoked by uid 89); 11 Dec 2015 15:06:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 11 Dec 2015 15:06:09 +0000 Received: from EUSAAHC001.ericsson.se (Unknown_Domain [147.117.188.75]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id 75.A8.06940.3D5EA665; Fri, 11 Dec 2015 16:03:48 +0100 (CET) Received: from [142.133.110.95] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.77) with Microsoft SMTP Server id 14.3.248.2; Fri, 11 Dec 2015 10:06:06 -0500 Subject: Re: [PATCH v7.1] Support software single step on ARM in GDBServer. To: Yao Qi , Pedro Alves References: <1449583641-18156-7-git-send-email-antoine.tremblay@ericsson.com> <1449691701-11845-1-git-send-email-antoine.tremblay@ericsson.com> <8637v9qc50.fsf@gmail.com> CC: From: Antoine Tremblay Message-ID: <566AE65E.5080209@ericsson.com> Date: Fri, 11 Dec 2015 15:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <8637v9qc50.fsf@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-12/txt/msg00235.txt.bz2 On 12/11/2015 09:43 AM, Yao Qi wrote: > Antoine Tremblay writes: > >> + /* Assume all atomic sequences start with a ldrex{,b,h,d} instruction. */ >> + insn1 = self->ops->read_memory_unsigned_integer (loc, 2, byte_order_for_code); >> + >> + loc += 2; >> + if (thumb_insn_size (insn1) != 4) >> + return NULL; >> + >> + insn2 = self->ops->read_memory_unsigned_integer (loc, 2, byte_order_for_code); >> + > > This line is too long, you may define a macro to shorten > "self->ops->read_memory_unsigned_integer". > This line is 79 long. From the GNU coding standard : "Please keep the length of source lines to 79 characters or less" So I think it's ok. I'm not sure a macro is a good thing, it often makes the code harder to parse for ides/emacs etc... And I don't think shortening the lines is a good justification in general for a macro. How about I use a function pointer variable like : ULONGEST (*read_memory_uint) (CORE_ADDR memaddr, int len, int byte_order); read_memory_uint = self->ops->read_memory_unsigned_integer; That would be already 23 shorter. >> + loc += 2; >> + if (!((insn1 & 0xfff0) == 0xe850 >> + || ((insn1 & 0xfff0) == 0xe8d0 && (insn2 & 0x00c0) == 0x0040))) >> + return NULL; >> + >> + /* Assume that no atomic sequence is longer than "atomic_sequence_length" >> + instructions. */ >> + for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count) >> + { >> + insn1 >> + = self->ops->read_memory_unsigned_integer (loc, 2,byte_order_for_code); >> + loc += 2; >> + >> + if (thumb_insn_size (insn1) != 4) >> + { >> + /* Assume that there is at most one conditional branch in the >> + atomic sequence. If a conditional branch is found, put a >> + breakpoint in its destination address. */ >> + if ((insn1 & 0xf000) == 0xd000 && bits (insn1, 8, 11) != 0x0f) >> + { >> + if (last_breakpoint > 0) >> + return NULL; /* More than one conditional branch found, >> + fallback to the standard code. */ >> + >> + breaks[1] = loc + 2 + (sbits (insn1, 0, 7) << 1); >> + last_breakpoint++; >> + } >> + >> + /* We do not support atomic sequences that use any *other* >> + instructions but conditional branches to change the PC. >> + Fall back to standard code to avoid losing control of >> + execution. */ >> + else if (thumb_instruction_changes_pc (insn1)) >> + return NULL; >> + } >> + else >> + { >> + insn2 = self->ops->read_memory_unsigned_integer >> + (loc, 2, byte_order_for_code); > > Format looks wrong, multiple instances of this problem in the patch. > Yes actually I was not sure about that and discussed this with Pedro and he agreed this was ok. That's why I went with that. At some point when you have if if if long_function_name (long variable, And that does not fit you could have long_function_name ( long variable, ... ) or long_function_name (long variable, ...) or ? I went with the latter after discussion with Pedro but I'm open to suggestions. Possibly the change to function pointers variables would make this moot but I think it may still happen. Thanks, Antoine