Hi Segher, Thanks for the review, > On 6 Nov 2016, at 22:09, Segher Boessenkool <seg...@kernel.crashing.org> > wrote: >
> On Sun, Nov 06, 2016 at 12:13:16PM -0800, Iain Sandoe wrote:
>> 2016-11-06 Iain Sandoe <i...@codesourcery.com>
>>
>> PR target/57438
>> * config/i386/i386.c (ix86_code_end): Note that we emitted code where
>> the
>> function might otherwise appear empty for picbase thunks.
>> (ix86_output_function_epilogue): If we find a zero-sized function
>> assume that
>> reaching it is UB and trap. If we find a trailing label append a nop.
>> * config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we find
>> a zero-sized function assume that reaching it is UB and trap. If we
>> find a
>> trailing label, append a nop.
>
> These lines are too long.
fixed.
>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index b0d2b64..326e2e9 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -30148,11 +30148,16 @@ rs6000_output_function_epilogue (FILE *file,
>> {
>> #if TARGET_MACHO
>> macho_branch_islands ();
>> - /* Mach-O doesn't support labels at the end of objects, so if
>> - it looks like we might want one, insert a NOP. */
>> +
>> + if (TARGET_MACHO)
>
> Both #if and if?
As discussed on IRC, I was under the impression that it is desired to move away
from #ifdef towards if() and I have been adding those where locally things
have been touched - in this case it was only partially possible.
However, FAOD - you pointed out that for the RS6000 back-end #ifdef are still
preferred, and I’ve removed this change.
> + /* Mach-O doesn't support labels at the end of objects, so if
>> + it looks like we might want one, take special action.
>> +
>> + First, collect any sequence deleted debug labels. */
>
> "sequence of”.
indeed
>
>> + /* If we have:
>> + label:
>> + barrier
>> + That need to be guarded. */
>
> "then this needs to be guarded.", or similar?
amended,
>
>> + /* up to now we've only seen notes or barriers. */
>
> Sentences start with a capital letter.
fixed
>
>> + /* See if have a completely empty function body. */
>
> "we have”
amended
>
>> + while (insn && ! INSN_P (insn))
>> + insn = PREV_INSN (insn);
>> + /* If we don't find any, we've got an empty function body; i.e.
>
> "any insn”?
amended
>
>> + completely empty - without a return or branch. GCC declares
>> + that reaching __builtin_unreachable() means UB (but we want
>> + finite-sized function bodies; to help the user out, let's trap
>> + the case. */
>
> Zero is finite size; "non-empty function bodies”?
yeah… amended.
>
> The rs6000 code looks fine, thanks; but please work on the text a bit :-)
Sorry, that was poor proof-reading (too many times looking at the same code, I
guess).
OK now for trunk?
Open branches?
Iain
PR target/57438
* config/i386/i386.c (ix86_code_end): Note that we emitted code
where the function might otherwise appear empty for picbase thunks.
(ix86_output_function_epilogue): If we find a zero-sized function
assume that reaching it is UB and trap. If we find a trailing label
append a nop.
* config/rs6000/rs6000.c (rs6000_output_function_epilogue): If we
find a zero-sized function assume that reaching it is UB and trap.
If we find a trailing label, append a nop.
PR57438-revised.patch
Description: Binary data
