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

[Jonathan Roelofs]

On 5/12/14, 5:13 PM, Reid Kleckner wrote:
> On Mon, May 12, 2014 at 4:44 PM, Nick Kledzik <kledzik at apple.com <mailto:kledzik at apple.com>> wrote:
>
>
>     On May 12, 2014, at 4:18 PM, Jonathan Roelofs <jonathan at codesourcery.com <mailto: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.
Attached is a patch that does this.
>
>
> 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.
I think this is ok because this case is expected to be very rare. I see Nick's suggestion (and the attached patch) as a "suggest that the guy who comes along later needing this does the right thing". 
  We really want to encourage people to do the one-function-per-file thing in general though, and this 'helps' that.

Jon

-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded
-------------- next part --------------
Index: lib/builtins/arm/sync_synchronize.S
===================================================================
--- lib/builtins/arm/sync_synchronize.S	(revision 208641)
+++ lib/builtins/arm/sync_synchronize.S	(working copy)
@@ -29,7 +29,4 @@
 	ldmfd	sp!, {r7, pc}
 END_COMPILERRT_FUNCTION(__sync_synchronize)
 
-	// tell linker it can break up file at label boundaries
-	.subsections_via_symbols
-		
 #endif
Index: lib/builtins/assembly.h
===================================================================
--- lib/builtins/assembly.h	(revision 208641)
+++ lib/builtins/assembly.h	(working copy)
@@ -23,19 +23,26 @@
 #endif
 
 #if defined(__APPLE__)
-#define HIDDEN_DIRECTIVE .private_extern
-#define LOCAL_LABEL(name) L_##name
-// tell linker it can break up file at label boundaries
-#define FILE_LEVEL_DIRECTIVE  .subsections_via_symbols
-#define SYMBOL_IS_FUNC(name)
+#  define HIDDEN_DIRECTIVE .private_extern
+#  define LOCAL_LABEL(name) L_##name
+#  define SYMBOL_IS_FUNC(name)
+#  if !defined(COMPILERRT_NON_STD_DARWIN)
+  /* Any assembly code that has multiple entry points needs to
+       #define COMPILERRT_NON_STD_DARWIN
+     before including assembly.h.
+
+     Otherwise, we tell the linker
+     that it can break up file at label boundaries:
+   */
+  .subsections_via_symbols
+#  endif
 #else
-#define HIDDEN_DIRECTIVE .hidden
-#define LOCAL_LABEL(name) .L_##name
-#define FILE_LEVEL_DIRECTIVE
+#  define HIDDEN_DIRECTIVE .hidden
+#  define LOCAL_LABEL(name) .L_##name
 #  if defined(__arm__)
-#  define SYMBOL_IS_FUNC(name) .type name, %function
+#    define SYMBOL_IS_FUNC(name) .type name, %function
 #  else
-#  define SYMBOL_IS_FUNC(name) .type name, @function
+#    define SYMBOL_IS_FUNC(name) .type name, @function
 #  endif
 #endif
 
@@ -97,7 +104,7 @@
 #endif
 
 #define DEFINE_COMPILERRT_FUNCTION(name)                   \
-  FILE_LEVEL_DIRECTIVE     SEPARATOR                       \
+  SYMBOL_IS_FUNC(name)     SEPARATOR                       \
   .globl SYMBOL_NAME(name) SEPARATOR                       \
   SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR              \
   DECLARE_SYMBOL_VISIBILITY(name)                          \
@@ -104,7 +111,6 @@
   SYMBOL_NAME(name):
 
 #define DEFINE_COMPILERRT_PRIVATE_FUNCTION(name)           \
-  FILE_LEVEL_DIRECTIVE     SEPARATOR                       \
   .globl SYMBOL_NAME(name) SEPARATOR                       \
   SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR              \
   HIDDEN_DIRECTIVE SYMBOL_NAME(name) SEPARATOR             \