Allow use of ^blocks, even with the 10.5 SDK, and even with a 10.5 runtime

Allow use of ^blocks, even with the 10.5 SDK, and even with a 10.5 runtime.

BUG=91978
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95892

[Mark Mentovai, 2011-08-08 18:30:21]

TVL for review, because it’s low-level build compilery linky stuff.

Nico’ll find that interesting, plus he’s going to want to use this, PLUS there
may be mac make AND clang impact.

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
File chrome/browser/mac/closure_leopard_compat/block.h (right):

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
chrome/browser/mac/closure_leopard_compat/block.h:26:
__attribute__((weak_import)) extern void* _NSConcreteGlobalBlock[32];
Nico: FYI: References to _NSConcreteGlobalBlock are compiler-emitted and don’t
really have anything to do with the declaration, and indeed, there is normally
no declaration in standard cases. This trick here is sufficient to get gcc-4.2
to treat it as a weak import, but it’s possible that clang behaves differently.
In that case, we’ll need to either fix clang or emit our own .weak_reference
directive in an asm block instead of doing this declaration with the weak_import
attribute.

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
File chrome/browser/mac/closure_leopard_compat/closure_leopard_compat.gyp
(right):

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
chrome/browser/mac/closure_leopard_compat/closure_leopard_compat.gyp:54:
'LD_DYLIB_INSTALL_NAME': '/usr/lib/libSystem.B.dylib',
Nico: FYI: The Mac make generator will need to support this. I’m not sure if it
does yet.

[Nico, 2011-08-08 18:43:51]

On 2011/08/08 18:30:21, Mark Mentovai wrote:
>
http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
> chrome/browser/mac/closure_leopard_compat/block.h:26:
> __attribute__((weak_import)) extern void* _NSConcreteGlobalBlock[32];
> Nico: FYI: References to _NSConcreteGlobalBlock are compiler-emitted and don’t
> really have anything to do with the declaration, and indeed, there is normally
> no declaration in standard cases. This trick here is sufficient to get gcc-4.2
> to treat it as a weak import, but it’s possible that clang behaves
differently.
> In that case, we’ll need to either fix clang or emit our own .weak_reference
> directive in an asm block instead of doing this declaration with the
weak_import
> attribute.

Add a small testcase that does "if (on 10.6) call closure". Then the waterfall
can tell us if this works with clang.

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
> File chrome/browser/mac/closure_leopard_compat/closure_leopard_compat.gyp
> (right):
> 
>
http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
> chrome/browser/mac/closure_leopard_compat/closure_leopard_compat.gyp:54:
> 'LD_DYLIB_INSTALL_NAME': '/usr/lib/libSystem.B.dylib',
> Nico: FYI: The Mac make generator will need to support this. I’m not sure if
it
> does yet.

It doesn't yet, it's http://crbug.com/90199

[TVL, 2011-08-08 18:50:20]

lgtm with some comment comments.  :)

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
File chrome/browser/mac/closure_leopard_compat/definitions.S (right):

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
chrome/browser/mac/closure_leopard_compat/definitions.S:24: # configurations.
I'd probably throw in here what we talked about on chat, that this also requires
building on 10.6 so the compiler has blocks support and the current toolchains
don't check the target sdk before doing the blocks codegen.

http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
chrome/browser/mac/closure_leopard_compat/definitions.S:53: # library are not
expected to treat these other symbols as weak imports.
so the goal is provide atleast one api that stays "strong" to the whole library
isn't a weak import?  (not completely spelled out, so just confirming)

[Mark Mentovai, 2011-08-08 19:13:36]

thakis@chromium.org wrote:
> On 2011/08/08 18:30:21, Mark Mentovai wrote:
>
http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
>>
>> chrome/browser/mac/closure_leopard_compat/block.h:26:
>> __attribute__((weak_import)) extern void* _NSConcreteGlobalBlock[32];
>> Nico: FYI: References to _NSConcreteGlobalBlock are compiler-emitted and
>> don’t
>> really have anything to do with the declaration, and indeed, there is
>> normally
>> no declaration in standard cases. This trick here is sufficient to get
>> gcc-4.2
>> to treat it as a weak import, but it’s possible that clang behaves
>
> differently.
>>
>> In that case, we’ll need to either fix clang or emit our own
>> .weak_reference
>> directive in an asm block instead of doing this declaration with the
>
> weak_import
>>
>> attribute.
>
> Add a small testcase that does "if (on 10.6) call closure". Then the
> waterfall
> can tell us if this works with clang.

That won’t really test what we need to test, though. Or at least it
won’t test what I was trying to bring to your attention.

My case: I need to make sure that the result is launchable on 10.5.
There isn’t much doubt that it’ll work on 10.6, and although I can add
a test for it, I’m much more interested in testing that the symbol
shows up as weak in the final linked output. The only way I can think
of to test that, short of running on 10.5, would be to walk the symbol
table and look for N_WEAK_REF. But we already have 10.5 testerbots, so
that should give enough coverage, right?

[Nico, 2011-08-08 19:30:15]

On Mon, Aug 8, 2011 at 12:13 PM, Mark Mentovai <mark@chromium.org> wrote:
> thakis@chromium.org wrote:
>> On 2011/08/08 18:30:21, Mark Mentovai wrote:
>>
http://codereview.chromium.org/7582032/diff/3001/chrome/browser/mac/closure_l...
>>>
>>> chrome/browser/mac/closure_leopard_compat/block.h:26:
>>> __attribute__((weak_import)) extern void* _NSConcreteGlobalBlock[32];
>>> Nico: FYI: References to _NSConcreteGlobalBlock are compiler-emitted and
>>> don’t
>>> really have anything to do with the declaration, and indeed, there is
>>> normally
>>> no declaration in standard cases. This trick here is sufficient to get
>>> gcc-4.2
>>> to treat it as a weak import, but it’s possible that clang behaves
>>
>> differently.
>>>
>>> In that case, we’ll need to either fix clang or emit our own
>>> .weak_reference
>>> directive in an asm block instead of doing this declaration with the
>>
>> weak_import
>>>
>>> attribute.
>>
>> Add a small testcase that does "if (on 10.6) call closure". Then the
>> waterfall
>> can tell us if this works with clang.
>
> That won’t really test what we need to test, though. Or at least it
> won’t test what I was trying to bring to your attention.
>
> My case: I need to make sure that the result is launchable on 10.5.
> There isn’t much doubt that it’ll work on 10.6, and although I can add
> a test for it, I’m much more interested in testing that the symbol
> shows up as weak in the final linked output. The only way I can think
> of to test that, short of running on 10.5,

Don't the bots actually run tests on 10.5?

> would be to walk the symbol
> table and look for N_WEAK_REF. But we already have 10.5 testerbots, so
> that should give enough coverage, right?
>

[Mark Mentovai, 2011-08-08 19:32:53]

Nico Weber wrote:
> Don't the bots actually run tests on 10.5?

Yeah, so that’ll tell us as soon as dyld tries to load libSystem into
a test binary whether the symbol was a weak import (good) or not (bad,
test won’t run, dyld will print an error).

We don’t need a new test for that, though, right? Assuming that the
other blocks-using code is tested.

I guess you just want a test specifically for this so that even if the
other blocks-using code doesn’t wind up being tested, we’re guaranteed
to have a test that checks just for this? That’s fair.

[Nico, 2011-08-08 19:36:46]

On Mon, Aug 8, 2011 at 12:32 PM, Mark Mentovai <mark@chromium.org> wrote:
> Nico Weber wrote:
>> Don't the bots actually run tests on 10.5?
>
> Yeah, so that’ll tell us as soon as dyld tries to load libSystem into
> a test binary whether the symbol was a weak import (good) or not (bad,
> test won’t run, dyld will print an error).
>
> We don’t need a new test for that, though, right? Assuming that the
> other blocks-using code is tested.

As of now, there's no other blocks-using code.

>
> I guess you just want a test specifically for this so that even if the
> other blocks-using code doesn’t wind up being tested, we’re guaranteed
> to have a test that checks just for this? That’s fair.

Yes.

[Mark Mentovai, 2011-08-08 20:28:56]

Ready for another look.

[Nico, 2011-08-08 21:03:40]

LGTM

Thanks for humoring me

http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
File chrome/browser/mac/closure_blocks_leopard_compat.S (right):

http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
chrome/browser/mac/closure_blocks_leopard_compat.S:27: #include
<AvailabilityMacros.h>
Just out of curiosity, why a .S file instead of a "normal" c file?

http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
File chrome/browser/mac/closure_blocks_leopard_compat.h (right):

http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
chrome/browser/mac/closure_blocks_leopard_compat.h:16: // blocks.
might want to link to a bug about switching to the 10.6 sdk in the medium term,
and explain in the bug why that's not trivial (it has to happen eventually after
all)

[Mark Mentovai, 2011-08-08 21:11:24]

thakis@chromium.org wrote:
>
http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
> chrome/browser/mac/closure_blocks_leopard_compat.S:27: #include
> <AvailabilityMacros.h>
> Just out of curiosity, why a .S file instead of a "normal" c file?

I only care about symbols here, not implementations. .s is the more
direct way to go. It’s a pain to define a symbol with a stupid $ in it
in .c. You can’t do that without asm. I figured, at that point, why
not just make the whole thing asm.

http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
> File chrome/browser/mac/closure_blocks_leopard_compat.h (right):
>
>
http://codereview.chromium.org/7582032/diff/7003/chrome/browser/mac/closure_b...
> chrome/browser/mac/closure_blocks_leopard_compat.h:16: // blocks.
> might want to link to a bug about switching to the 10.6 sdk in the
> medium term, and explain in the bug why that's not trivial (it has to
> happen eventually after all)

It may not happen until we drop 10.5, even if the technical ability is there…

Seriously, when I was deciding between doing something like this
change and switching the SDK, I decided that I really liked having the
protection of not having anything in 10.6 but not 10.5 declared.

[Mark Mentovai, 2011-08-08 22:14:35]

I did some local testing with clang and found that it respects the user-provided
declaration of _NSConcrete{Global,Stack}Block the same way that GCC does. I
added a comment mentioning this.