[ORC] Fix macho section name typo

[keith]

Summary

I don't think obj_selrefs is a thing, but objc_selrefs definitely
is.

[lhames]

LGTM -- thanks @keith!

[dblaikie]

Any chance of test coverage?

[keith]

I poked around for an hour or so trying to get something working here, but I'm not familiar with how these types fit together at all, so I couldn't come up with something for this. (I just spotted this typo looking around for other uses of __objc_selrefs in general). I was attempting to write a test where I added one of these sections to the link graph, and then looked up the init symbol that should be added, any tips for how to test this would be appreciated!

[dblaikie]

In D130221#3802866, @keith wrote:

I poked around for an hour or so trying to get something working here, but I'm not familiar with how these types fit together at all, so I couldn't come up with something for this. (I just spotted this typo looking around for other uses of __objc_selrefs in general). I was attempting to write a test where I added one of these sections to the link graph, and then looked up the init symbol that should be added, any tips for how to test this would be appreciated!

Assuming some of this code is already tested, one way to find the existing test coverage would be to remove this whole for+if and let the function always return false - then run the llvm (& maybe compiler-rt) tests and see what fails - hopefully something fails, and that'll point you to tests that exercise this code. If that doesn't turn up anything, because the tests are possibly underconstrained, you could try adding an assertion that the name isn't any of the ones in this list - then at least if a test exercises any of these names you'd hit an assertion and could figure out if the behavior was observable...

[keith]

Yea the only tests around this code I found are https://github.com/llvm/llvm-project/blob/f2949febf354eb8aded7db3e63d2c878fe86db3b/llvm/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp which aren't a ton of coverage. From the initial commit of this logic https://github.com/llvm/llvm-project/commit/0fda4c4745b81e8a0eed2b80b0b03f33c16c2b99 my assumption is no other tests are covering this today

[dblaikie]

In D130221#3803469, @keith wrote:

Yea the only tests around this code I found are https://github.com/llvm/llvm-project/blob/f2949febf354eb8aded7db3e63d2c878fe86db3b/llvm/unittests/ExecutionEngine/Orc/ObjectLinkingLayerTest.cpp which aren't a ton of coverage. From the initial commit of this logic https://github.com/llvm/llvm-project/commit/0fda4c4745b81e8a0eed2b80b0b03f33c16c2b99 my assumption is no other tests are covering this today

Looks like that original commit does have source coverage of this function (adding an assert(false) fails only that unit test) but the behavior is not verified (hardcoding the function's return (to either true or false)).

@lhames - is there some way this functionality should be observable in that unit test? Or covered in some other way?

[dblaikie]

Ping

[dblaikie]

Ping

[dblaikie]

Ping

[dblaikie]

Ping

[lhames]

Sorry -- I'm not getting Phab pings for some reason. I'll have to look into that.

@lhames - is there some way this functionality should be observable in that unit test? Or covered in some other way?

I was attempting to write a test where I added one of these sections to the link graph, and then looked up the init symbol that should be added, any tips for how to test this would be appreciated!

I think there's an easy test with dubious value, and a valuable but awkward/ test:

Easy but dubious: create LinkGraphs that contain each of the init sections (but none of the others), create materialization units for each of those graphs, then assert that each MU has an init symbol. The problem is that the list you use to create the graphs would usually be a copy/paste of the original, which wouldn't have found this bug. I'm not sure how much value there is in that.

Valuable but awkward: Develop an API for creating LinkGraphs directly from text representations (Obj2LinkGraph?) and then write an extra regression test tool for the ORC runtime (or add functionality to llvm-jitlink) to verify that a graph containing an objc selrefs function behaves as expected at runtime if you call a contained selector. Since constructing graphs via API calls is rare at the moment I'm not sure this is worth the effort either.

Non-testing solution: Replace this with MachOPlatform::isInitializerSection (which is easily testable as an ORC runtime regression test) -- no function, no need for a test. ;)

[dblaikie]

In D130221#3942720, @lhames wrote:

Sorry -- I'm not getting Phab pings for some reason. I'll have to look into that.

@lhames - is there some way this functionality should be observable in that unit test? Or covered in some other way?

I was attempting to write a test where I added one of these sections to the link graph, and then looked up the init symbol that should be added, any tips for how to test this would be appreciated!

I think there's an easy test with dubious value, and a valuable but awkward/ test:

Easy but dubious: create LinkGraphs that contain each of the init sections (but none of the others), create materialization units for each of those graphs, then assert that each MU has an init symbol. The problem is that the list you use to create the graphs would usually be a copy/paste of the original, which wouldn't have found this bug. I'm not sure how much value there is in that.

This is all I was looking for - and I think at least of moderate value/worth writing - not that it would have found this bug ahead of time, but that it would document the expected behavior/avoid regressions (eg: if this function got refactored to use MachOPlatform::isInitializerSection and that function ended up with different behavior because of it (though, admittedly, if it'd been written in terms of that function in the first place, I wouldn't have expected exhaustive testing here too - if the function was tested elsewhere, that'd be sufficient for me). That's what I've understood to be some of the benefits of the ORC architecture - that it's got more separate pieces that can be API tested directly without having to necessarily run a live process, etc, which might not be possible to cover everything on a variety of test machines, etc.

Is there already some API test coverage for functionality in this area that this could be added to so it's not so much setup for relatively trivial testing?

Seems like there's enough complexity in the ObjectLinkingLayer that it's probably got some existing coverage this could be added to/grouped with - that'd test not only this function in isolation, but as you say, the interaction with creating the init symbol in scanLinkGraph, and is maybe already/could be testing other features of that code.

Valuable but awkward: Develop an API for creating LinkGraphs directly from text representations (Obj2LinkGraph?) and then write an extra regression test tool for the ORC runtime (or add functionality to llvm-jitlink) to verify that a graph containing an objc selrefs function behaves as expected at runtime if you call a contained selector. Since constructing graphs via API calls is rare at the moment I'm not sure this is worth the effort either.

How's this ^ suggestion compare to testing for, say, the __swift5_types section here: https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/orc/TestCases/Darwin/x86-64/trivial-swift-types-section.S ? I don't know enough about the Swift/ObjC metadata things, but would it be possible to end-to-end test the selrefs feature in a similar way? (I think this would/should be in addition to the more unit-test-y/API-testy thing described in (1) so that there's some test coverage there for folks just working in llvm alone, without compiler-rt)

Non-testing solution: Replace this with MachOPlatform::isInitializerSection (which is easily testable as an ORC runtime regression test) -- no function, no need for a test. ;)

Always good to share common implementations - though if this patch had made that change I'd still have wanted some test coverage to demonstrate how that it fixed the bug/covered the new case. And it seems MachOPlatform::isInitializerSection is undertested too? I can change it to return false unconditionally and check-llvm passes, though if I add assert(false) in there, a 35 tests in ExecutionEngine/JITLink fail - so they're exercising it, but aren't testing the functionality.

[keith]

I submitted https://reviews.llvm.org/D139215 and https://reviews.llvm.org/D139223 based on this discussion. Please check them out!

[dblaikie]

In D130221#3967496, @keith wrote:

I submitted https://reviews.llvm.org/D139215 and https://reviews.llvm.org/D139223 based on this discussion. Please check them out!

Thanks! Yeah, D139223 seems like a pretty good direction to me. I probably wouldn't bother with D139215 - as it's a bit too simplified/trivial at that level.

[lhames]

Back from vacation next week, but quick responses:

Easy but dubious: create LinkGraphs that contain each of the init sections (but none of the others), create materialization units for each of those graphs, then assert that each MU has an init symbol. The problem is that the list you use to create the graphs would usually be a copy/paste of the original, which wouldn't have found this bug. I'm not sure how much value there is in that.

This is all I was looking for - and I think at least of moderate value/worth writing - not that it would have found this bug ahead of time, but that it would document the expected behavior/avoid regressions (eg: if this function got refactored to use MachOPlatform::isInitializerSection and that function ended up with different behavior because of it (though, admittedly, if it'd been written in terms of that function in the first place, I wouldn't have expected exhaustive testing here too - if the function was tested elsewhere, that'd be sufficient for me). That's what I've understood to be some of the benefits of the ORC architecture - that it's got more separate pieces that can be API tested directly without having to necessarily run a live process, etc, which might not be possible to cover everything on a variety of test machines, etc.

Is there already some API test coverage for functionality in this area that this could be added to so it's not so much setup for relatively trivial testing?

The LinkGraphTests.cpp unit test would be the right place if we went that way, but I don't think it's worth it. See below.

Valuable but awkward: Develop an API for creating LinkGraphs directly from text representations (Obj2LinkGraph?) and then write an extra regression test tool for the ORC runtime (or add functionality to llvm-jitlink) to verify that a graph containing an objc selrefs function behaves as expected at runtime if you call a contained selector. Since constructing graphs via API calls is rare at the moment I'm not sure this is worth the effort either.

How's this ^ suggestion compare to testing for, say, the __swift5_types section here: https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/orc/TestCases/Darwin/x86-64/trivial-swift-types-section.S ? I don't know enough about the Swift/ObjC metadata things, but would it be possible to end-to-end test the selrefs feature in a similar way? (I think this would/should be in addition to the more unit-test-y/API-testy thing described in (1) so that there's some test coverage there for folks just working in llvm alone, without compiler-rt)

Those are testing LinkGraphs created from object files which are using MachOPlatform::isInitializerSection to create the init symbol.

Non-testing solution: Replace this with MachOPlatform::isInitializerSection (which is easily testable as an ORC runtime regression test) -- no function, no need for a test. ;)

Always good to share common implementations - though if this patch had made that change I'd still have wanted some test coverage to demonstrate how that it fixed the bug/covered the new case. And it seems MachOPlatform::isInitializerSection is undertested too? I can change it to return false unconditionally and check-llvm passes, though if I add assert(false) in there, a 35 tests in ExecutionEngine/JITLink fail - so they're exercising it, but aren't testing the functionality.

These are tested in the ORC runtime, since the runtime is needed for this functionality to work.

So the right answer is to unify MachOPlatform::isInitializerSection and hasMachOInitSection, and then make sure there's an ORC runtime regression test that exercises each of the special sections.

[dblaikie]

In D130221#3968054, @lhames wrote:

Back from vacation next week, but quick responses:

Easy but dubious: create LinkGraphs that contain each of the init sections (but none of the others), create materialization units for each of those graphs, then assert that each MU has an init symbol. The problem is that the list you use to create the graphs would usually be a copy/paste of the original, which wouldn't have found this bug. I'm not sure how much value there is in that.

This is all I was looking for - and I think at least of moderate value/worth writing - not that it would have found this bug ahead of time, but that it would document the expected behavior/avoid regressions (eg: if this function got refactored to use MachOPlatform::isInitializerSection and that function ended up with different behavior because of it (though, admittedly, if it'd been written in terms of that function in the first place, I wouldn't have expected exhaustive testing here too - if the function was tested elsewhere, that'd be sufficient for me). That's what I've understood to be some of the benefits of the ORC architecture - that it's got more separate pieces that can be API tested directly without having to necessarily run a live process, etc, which might not be possible to cover everything on a variety of test machines, etc.

Is there already some API test coverage for functionality in this area that this could be added to so it's not so much setup for relatively trivial testing?

The LinkGraphTests.cpp unit test would be the right place if we went that way, but I don't think it's worth it. See below.

Valuable but awkward: Develop an API for creating LinkGraphs directly from text representations (Obj2LinkGraph?) and then write an extra regression test tool for the ORC runtime (or add functionality to llvm-jitlink) to verify that a graph containing an objc selrefs function behaves as expected at runtime if you call a contained selector. Since constructing graphs via API calls is rare at the moment I'm not sure this is worth the effort either.

How's this ^ suggestion compare to testing for, say, the __swift5_types section here: https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/orc/TestCases/Darwin/x86-64/trivial-swift-types-section.S ? I don't know enough about the Swift/ObjC metadata things, but would it be possible to end-to-end test the selrefs feature in a similar way? (I think this would/should be in addition to the more unit-test-y/API-testy thing described in (1) so that there's some test coverage there for folks just working in llvm alone, without compiler-rt)

Those are testing LinkGraphs created from object files which are using MachOPlatform::isInitializerSection to create the init symbol.

Non-testing solution: Replace this with MachOPlatform::isInitializerSection (which is easily testable as an ORC runtime regression test) -- no function, no need for a test. ;)

Always good to share common implementations - though if this patch had made that change I'd still have wanted some test coverage to demonstrate how that it fixed the bug/covered the new case. And it seems MachOPlatform::isInitializerSection is undertested too? I can change it to return false unconditionally and check-llvm passes, though if I add assert(false) in there, a 35 tests in ExecutionEngine/JITLink fail - so they're exercising it, but aren't testing the functionality.

These are tested in the ORC runtime, since the runtime is needed for this functionality to work.

So the right answer is to unify MachOPlatform::isInitializerSection and hasMachOInitSection, and then make sure there's an ORC runtime regression test that exercises each of the special sections.

*nod* I'd hope there's some test coverage in the llvm subproject too - even if it's a bit narrower, necessarily. (same way we don't rely on clang or lld tests to test LLVM functionality, but isolate/test it directly too) But yeah, good to have the end-to-end/feature/scenario testing in the runtime testing too.

(aside: Does the MacOS linker really have a special cased list of these sections that gets updated whenever a new one is added? Rather than any kind of section flag/metadata? That's a bit surprising to me (but again, comes back to the 'smart format + dumb linker' V 'dumb format + smart linker' thing between Unix & MacOS, I guess)

[lhames]

So the right answer is to unify MachOPlatform::isInitializerSection and hasMachOInitSection, and then make sure there's an ORC runtime regression test that exercises each of the special sections.

*nod* I'd hope there's some test coverage in the llvm subproject too - even if it's a bit narrower, necessarily. (same way we don't rely on clang or lld tests to test LLVM functionality, but isolate/test it directly too) But yeah, good to have the end-to-end/feature/scenario testing in the runtime testing too.

There are plenty of JIT unit and regression tests in LLVM (and the ORC runtime), but the init symbol stuff sits at the interface between LLVM and the ORC runtime so the best value-for-money testing (of init symbols, and Platform implementations more generally) is in regression testing with the ORC runtime. I don't mind the trivial unit test being added, but the regression test coverage is my priority.

(aside: Does the MacOS linker really have a special cased list of these sections that gets updated whenever a new one is added? Rather than any kind of section flag/metadata? That's a bit surprising to me (but again, comes back to the 'smart format + dumb linker' V 'dumb format + smart linker' thing between Unix & MacOS, I guess)

Even better: It's a mix of types and special section names. E.g. thread local variable sections are identifiable by type, but language runtime sections just have recognized names. ;)

[dblaikie]

In D130221#3969515, @lhames wrote:

So the right answer is to unify MachOPlatform::isInitializerSection and hasMachOInitSection, and then make sure there's an ORC runtime regression test that exercises each of the special sections.

*nod* I'd hope there's some test coverage in the llvm subproject too - even if it's a bit narrower, necessarily. (same way we don't rely on clang or lld tests to test LLVM functionality, but isolate/test it directly too) But yeah, good to have the end-to-end/feature/scenario testing in the runtime testing too.

There are plenty of JIT unit and regression tests in LLVM (and the ORC runtime), but the init symbol stuff sits at the interface between LLVM and the ORC runtime so the best value-for-money testing (of init symbols, and Platform implementations more generally) is in regression testing with the ORC runtime. I don't mind the trivial unit test being added, but the regression test coverage is my priority.

*nod* for sure, if I had to pick one, I'd take the runtime feature testing for sure. Appreciate the pointers you've provided on how best to do the unit testing too. Thanks!

(aside: Does the MacOS linker really have a special cased list of these sections that gets updated whenever a new one is added? Rather than any kind of section flag/metadata? That's a bit surprising to me (but again, comes back to the 'smart format + dumb linker' V 'dumb format + smart linker' thing between Unix & MacOS, I guess)

Even better: It's a mix of types and special section names. E.g. thread local variable sections are identifiable by type, but language runtime sections just have recognized names. ;)

Delightful! :s

llvm/lib/ExecutionEngine/Orc/ObjectLinkingLayer.cpp

if ((G.getTargetTriple().isOSBinFormatMachO() && hasMachOInitSection(G)) ||
         (G.getTargetTriple().isOSBinFormatELF() && hasELFInitSection(G)))
       LGI.InitSymbol = makeInitSymbol(ES, G);
 
     return LGI;
   }
 
   static bool hasMachOInitSection(LinkGraph &G) {
     for (auto &Sec : G.sections())
-      if (Sec.getName() == "__DATA,__obj_selrefs" ||
+      if (Sec.getName() == "__DATA,__objc_selrefs" ||
           Sec.getName() == "__DATA,__objc_classlist" ||
           Sec.getName() == "__TEXT,__swift5_protos" ||
           Sec.getName() == "__TEXT,__swift5_proto" ||
           Sec.getName() == "__TEXT,__swift5_types" ||
           Sec.getName() == "__DATA,__mod_init_func")
         return true;
     return false;
   }