This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] ARM: Add support for SHF_ARM_NOREAD section flag


On 08/01/16 13:00, Christophe MONAT wrote:
> Hi Andre, Nick,
> 
> [Sorry that Mickael does not answer by himself, he was not subscribed to
> the list so far -- now corrected].
> 
> On 01/07/16 19:33, Andre Vieira wrote:
>> Terry sent a patch upstream to handle the noread attribute in 2014:
>> https://sourceware.org/ml/binutils/2014-04/msg00181.html
>>
>> Having seen this patch I believe the approach taken here to use section
>> names to represent the noread attribute in assembly is inferior to
>> Terry's approach.
>>
>> For the GCC implementation of either an attribute or compile option for
>> execute-only we should not use section names to represent the noread
>> attribute, since for instance that means it can not be combined with
>> -ffunction-sections, or any other option that sets section names for
>> functions.
> 
> We disagree with that specific point : the section names that we emit
> when gcc is using -ffunction-sections is in the form of:
> .text.noread.*
> which are perfectly caught and handled (it matters to us - we just
> checked this).
> 
>> I would like to rebase Terry's patch and make the necessary changes to
>> it, slightly different attribute name and so on, and use that instead of
>> this patch.
>>
>> Would there be any objections to this?
> 
> The binutils patch that we contributed was in the perspective of
> up-streaming the so-called PCROP support also in gcc also - the gcc
> proposal is completed on our side, but still not public.
> 
> For the matter of marking the .text sections read-only, we tried the two
> following strategies (and choose 2)):
> 
> 1) keep the .text sections' names and emit the noread attribute in the
> assembly (with the very same 'y' key) : we failed doing that because of
> the specific treatment done by gas, that in the end *ignores* at some
> final point the custom attributes on the pure .text name.
> This implies that the .text sections in noread mode cannot be called
> .text but must be called .text.something (which in your patch's tests
> appears as .text.foo, otherwise the noread attribute would not been have
> accounted for).
> In addition from the gcc standpoint, this emission forces to duplicate
> emission code since the particular place we need to touch is hook-able,
> but requires a complete duplication of the section attribute emission
> (default_elf_asm_named_section) just to add two lines dealing with the
> 'y' case.
> 
> 2) emit some .text.noread sections (that work nicely with
> -ffunction-sections), without even requiring a section attribute change,
> and delegating the treatment to gas that handles specifically those
> sections.
> 
> This to write that your patch is certainly very good (just regretting
> that it was not contributed at that time) but leads to uglier code in
> gcc, which you may consider a problem.
> 
> Would it be possible to simply add the 'y' support from your patch, this
> would not break our changes and fulfill your purposes ?
> 

Just a quick comment, I don't currently have time to dig into this deeply.

My main concern is that requiring a specific template in the section
name feels like a cludge.  If gas/ld is not correctly dealing with the
section flags, then that is a bug that ought to be fixed properly.
Working around that by inventing new special meta-data is not helpful --
don't forget that other linkers besides GNU LD have to be able to
process these sections (and GNU LD has to process object files produced
by tools other than GCC/GAS).

R.

> Best regards,
> --C


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]