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

Tue May 13 18:11:50 PDT 2014

On Mon, May 12, 2014 at 6:50 PM, Nick Kledzik <kledzik at apple.com> wrote:

>
> 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.
>

This seems reasonable to me.

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

-- 
Saleem Abdulrasool
compnerd (at) compnerd (dot) org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140513/7304fd4c/attachment.html>