On 12/23/2010 12:02 PM, Joel Brobecker wrote:
> 
>> +/* Extract the immediate from instruction movw/movt of encoding T.  INSN1 is the
>> +   first 16-bit of instruction, and INSN2 is the second 16-bit of
>> +   instruction.  */
> 
> The first line is a little too long. It's exactly 80 characters, but
> I would prefer if it didn't exceed 78.  I occasionally break that rule
> when the extra 2 characters allow the code formatting to be less ugly,
> but this is not the case here.
> 

OK.  Done.

>> +/* Extract the immediate form instruction movw/movt of encoding A.  INSN is
>                             ^^^^ spelling `from'

Fixed this typo.

> 
> Just on a personal suggestion that you don't have to agree with, I find:
> 
>> -	      unsigned int imm = ((bits (insn, 0, 3) << 12)
>> -				  | (bits (insn, 10, 10) << 11)
>> -				  | (bits (inst2, 12, 14) << 8)
>> -				  | bits (inst2, 0, 7));
> 
> ... more easily readable than the two-line form you used in the macro.
> 

Yeah, I agree.  Done.

>>  
>> +
>> +/* Try to analyze the the instructions starting from PC, which load symbol
>                      ^^^^^^^

Sorry for this.  Fixed.

>> +  else
>> +    {
>> +       unsigned int insn
>> +	 = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
>> +       if ((insn & 0x0e5f0000) == 0x041f0000) /* ldr Rd, #immed */
> 
> Empty line after local variable declaration.
> 

Fixed with other instances.

>> +/* Try to skip a sequence of instructions used for stack protector.  If PC
>> +   points to the first instruction of this sequence, return the address of first
>> +   instruction after this sequence, otherwise, return original PC.
>> +
>> +   On arm, this sequence of instructions is composed by mainly three steps,
>                                                         ^^ of

Fixed.

>> +     Step 1: load symbol __stack_chk_guard,
>> +     Step 2: load from address of __stack_chk_guard,
>> +     Step 3: store it to somewhere else.
>> +
>> +   Usually, instructions on step 2 and step 3 are the same on various ARM
>> +   archtectures.  On step 2, it is one instruction 'ldr Rx, [Rn, #0]', and
       ^^^^^^^^^^^^
Fixed another typo here.

>> +   on step 3, it is also one instruction 'str Rx, [r7, #immd]'.  However,
>> +   instructions in step 1 varies from different ARM archs.  On ARMv7, they are,
>                              ^^^^^^ vary (plural)
> 
> I am not sure that "from different ARM archs" is correct English (arches,
> btw).  Suggest "from arch to arch" (we know that we're dealing with ARM
> only in this file). Perhaps "from ARM arch to arch" is also good English
> but I'm not sure.

I mean "different ARM architectures" are ARMv7, ARMv5t, ARMv6, and so
forth.  On wikipedia, they are called "ARM Architecture"
http://en.wikipedia.org/wiki/ARM_architecture

So, I s/archs/architectures here.
> 
>> +   Since ldr/str is a very popular instruction, we can't use them as
>> +   'fingerprint' or 'signature' of stack protector sequence.  Here we choose
>> +   sequence {movw/movt, ldr}/ldr/str plus symbol __stack_chk_guard, if not
>> +   stripped, as 'fingerprint' of stack protector cdoe sequence.  */
>                   ^^ the          ^^ a             ^^^^^ code

Fixed.

> 
>> +static CORE_ADDR
>> +arm_skip_stack_protector(CORE_ADDR pc, struct gdbarch *gdbarch)
>> +{
>> +  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> [...]
>> +  int is_thumb = arm_pc_is_thumb (gdbarch, pc);
>> +
>> +  /* Try to parse the instructions in Step 1.  */
>> +  CORE_ADDR addr = arm_analyze_load_stack_chk_guard (pc, gdbarch,
>> +						     &basereg, &offset);
>> +  if (!addr)
>> +    return pc;
> 
> This is a situation where we set ourselves up for adding code between
> the two groups of local variable declarations.  We've done that in
> the past...  I think that compilers detect that, now, so it's not so
> bad.
> 
> I suggest that you move the declaration of variable `addr' to the
> first group of declarations, and then assign it later.  Or you'll
> also need to add an empty line after its declaration.

Fixed by putting declaration of variable `addr' to the firs group.

> 
>> +  stack_chk_guard = lookup_minimal_symbol_by_pc (addr);
>> +  /* If name of symbol doesn't start with '__stack_chk_guard', this
>> +     instruction sequence is not for stack protector.  If symbol is
>> +     removed, we conservatively think this sequence is for stack protector.  */
>> +  if (stack_chk_guard
>> +      && strncmp (SYMBOL_NATURAL_NAME(stack_chk_guard), "__stack_chk_guard",
>> +		  strlen ("__stack_chk_guard")))
> 
> Why are you using the "natural" name rather than the linkage name?

In short, we should use linkage name here.  Since __stack_chk_guard is
added by GCC during compilation without any information on language, the
"natural" name is the same as linkage name in this case.  Fixed.

> Also, what would happen if the symbol name was __stack_chk_guard_1?
> I presume that you'll still fail the instruction match later one,
> but isn't a simple strcmp more accurate?

Replaced strncmp with strcmp.

> 
>> +  if (is_thumb)
>> +    {
>> +      unsigned int destreg;
>> +      unsigned short insn
>> +	= read_memory_unsigned_integer (pc + offset, 2, byte_order_for_code);
>> +      /* Step 2: ldr Rd, [Rn, #immed], encoding T1.  */
> 
> Empty line after variable decls.
> 

Done.

>> +  else
>> +    {
>> +      unsigned int destreg;
>> +      unsigned int insn
>> +	= read_memory_unsigned_integer (pc + offset, 4, byte_order_for_code);
>> +      /* Step 2: ldr Rd, [Rn, #immed], encoding A1.  */
> 
> Same here.
> 

Done.

>> +      if (post_prologue_pc)
>> +	{
>> +	  post_prologue_pc
>> +	    = arm_skip_stack_protector (post_prologue_pc, gdbarch);
>> +	}
> 
> We do not use the curly braces for single-instruction blocks:
> 
>         if (post_prologue_pc)
>           post_prologue_pc
>             = arm_skip_stack_protector (post_prologue_pc, gdbarch);
> 

Fixed.

-- 
Yao (齐尧)