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

Fri Dec 2 16:59:47 PST 2022

lhames added a comment.

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 `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)

Those are testing `LinkGraph`s 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130221