[compiler-rt] r208603 - Move .subsections_via_symbols directives into DEFINE_COMPILERRT_PRIVATE_FUNCTION

[Nick Kledzik]

On May 12, 2014, at 5:45 PM, Saleem Abdulrasool <compnerd at compnerd.org> wrote:

> On Mon, May 12, 2014 at 5:30 PM, Nick Kledzik <kledzik at apple.com> wrote:
> 
> On May 12, 2014, at 5:13 PM, Reid Kleckner <rnk at google.com> wrote:
> 
>> On Mon, May 12, 2014 at 4:44 PM, Nick Kledzik <kledzik at apple.com> wrote:
>> 
>> On May 12, 2014, at 4:18 PM, Jonathan Roelofs <jonathan at codesourcery.com> wrote:
>> 
>> >
>> >
>> > On 5/12/14, 3:47 PM, Nick Kledzik wrote:
>> >> FYI, this is a semantic change.   The .subsection_via_symbols directive tells the linker that the .o file has no functions that will fall into the next one (the compiler never does that, but it is common enough in hand written assembly).  That means the linker can dead strip functions not referenced.  By moving .subsection_via_symbols into FILE_LEVEL_DIRECTIVE, you’ve caused every file to opt-in to the directive, even if they have fall through code.
>> >>
>> > I think the real gotcha here is that DEFINE_COMPILERRT_FUNCTION, which defines something that looks like function level scope had/has a global directive in it affecting file level scope…
>> Yes!
>> 
>> >> I just did a quick survey of the .S files in compiler-rt and do not see any with fall through code.  Mostly because compiler-rt has the convention of one function per file.
>> > Does it really though? I didn't move .subsection_via_symbols into FILE_LEVEL_DIRECTIVE, it was already there. This only changes it for private functions, and further it makes those behave the same way as non-private ones, which already had it.
>> Oh, sorry I was looking at the final result and did not realize it was half broke before your change.
>> 
>> >>
>> >> So, this change should be benign now, but may cause surprises on darwin in the future if someone tries to write fall-through assembly code.
>> > So to eliminate this gotcha, it sounds like you are suggesting that I:
>> >  a) Remove FILE_LEVEL_DIRECTIVE from DEFINE_COMPILERRT_FUNCTION and DEFINE_COMPILERRT_PRIVATE_FUNCTION
>> >  b) Maybe also remove .subsections_via_symbols from FILE_LEVEL_DIRECTIVE
>> >  c) Add .subsections_via_symbols to every *.S file (possibly inside some other appropriately named macro)
>> >
>> > I'm happy to do this, but I want to make sure I'm clear on what you want.
>> That would be a clean solution, but it is a bunch of work to support a case we don’t have and probably don’t want.  A simpler solution would be:
>> a) Remove use of FILE_LEVEL_DIRECTIVE from DEFINE_COMPILERRT_FUNCTION and DEFINE_COMPILERRT_PRIVATE_FUNCTION
>> b) Remove definition of FILE_LEVEL_DIRECTIVE
>> c) Having assembly.h out right use  .subsections_via_symbols , e.g.:
>> 
>> #if defined(__APPLE__) && !defined(COMPILERRT_NON_STD_DARWIN)
>>   /* Any assembly code that has multiple entry points needs to
>>        #define COMPILERRT_NON_STD_DARWIN
>>     before including assembly.h
>>   */
>>   .subsections_via_symbols
>> #endif
>> 
>> So, by default it assumes all code follows the darwin conventions (as it currently does), but if an issue arises, there is an easy way to disable   .subsections_via_symbols on a file-by-file basis.
>> 
>> I'm not a compiler-rt person, but I don't think it's good design for headers to expect includers to define macros before inclusion.  I think the current situation is fine.
> 
> Currently LLVM requires __STDC_LIMIT_MACROS and __STD_CONSTANT_MACROS to be defined before some inclusions.  So, the idea is not without precedent...
> 
> .subsections_via_symbols is supposed to be opt-in.  The current state forces the opt-in with no way to opt-out.  My proposal is to opt-in by default (cause it is generally a good thing).  But it some problem arises, give the .S file a way to opt-out be defining COMPILERRT_NON_STD_DARWIN before including assembly.h.    I really hope no one ever uses COMPILERRT_NON_STD_DARWIN.
> 
> This would certainly be the solution with the least amount of work.
> 
> Unfortunately, there are exceptions to the general behavior of one function per file (c.f. compares2f.S in lib/builtins/arm).
Having multiple functions is compatible with .subsections_via_symbols (the compiler does that all the time).   It is having multiple entry points (one function falls into the next) that is problematic. 

> 
> Having a separate macro for this and including it in explicitly is much clearer and conscious, but it does leave the possibility of someone accidentally leaving it off.  That said, CompilerRT is generally pretty stable and doesn't see much churn in the builtins.
> 
> It sounded like your preference (it felt as though objection might be too strong a term here) was mostly due to amount of effort involved.  I do agree that it would be preferable to not actively encourage people to not use .subsections_via_symbols, but, are reviews insufficient to prevent that?
If someone adds a new file that *has* something (e.g. COMPILERRT_NON_STD_DARWIN), it is easy to catch in a review.  But if someone adds a file that is *missing* something, it is much easier to go unnoticed during a review (e.g. forgetting some new COMPILER_RT_NEW_ONCE_PER_FILE macro that does nothing on other platforms) .  

I’m trying to make the easy/normal path easy, and the special case path more complicated.

-Nick


> 
> 
> -- 
> Saleem Abdulrasool
> compnerd (at) compnerd (dot) org

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140512/2643ae07/attachment.html>