[PATCH] D130221: [ORC] Fix macho section name typo

Tue Nov 22 10:20:37 PST 2022

dblaikie added a comment.

In D130221#3942720 <https://reviews.llvm.org/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 `LinkGraph`s 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130221/new/

https://reviews.llvm.org/D130221