erichkeane added a comment.
In D112349#3116457 <https://reviews.llvm.org/D112349#3116457>, @ibookstein
wrote:
>> To me the 'not in the weeds' part is, "How do I get
>> CPU-dispatch/CPU-specific to work without RAUW, since that is offensive to
>> the CFE code owner? Additionally, I found that some of the examples without
>> the defined resolver actually DO work in my downstream, though I don't know
>> what changes we make to make that happen. So adding this limitation in
>> actually breaks my downstream.
>
> Regarding the examples that do work, I've provided explanations as to how
> they partially sort-of-work, but that the general semantics are broken (the
> 'connection' to the resolver is 'severed' in each translation unit that has
> it as a declaration + all references from within that TU are borked).
As I mentioned, my downstream actually DOES work, I believe our code-generator
has some changes that are making this work, but I haven't found the actual
commit yet. I suspect it is some level of changes that emit ifuncs with
undefined resolvers differently.
> The problems that the CFE currently uses RAUW to solve are //fundamental// to
> the specified behavior of the **language features** that use it, so a
> solution for these problems needs to arise from the CFE. Imagine the
> following incremental compilation use-case:
>
> > extern int x; // x is a GlobalVariable declaration (initializer == null).
> > int y; // y is a GlobalVariable definition (initializer = constant 0).
> > extern int x __attribute__((alias("y"))); // x needs to transform into a
> GlobalAlias now.
> >
> > extern void *my_memcpy(void *dest, void *src, size_t n); // my_memcpy is
> a Function declaration
> > void *my_memcpy(void *dest, void *src, unsigned long n)
> __attribute__((ifunc("my_memcpy_resolver"))); // my_memcpy_resolver is a
> Function declaration, my_memcpy is a GlobalIFunc with declaration for a
> resolver - whoops, incremental module is invalid at this point
> > static void *my_memcpy_impl(void *dest, void *src, unsigned long n) {
> return 0; }
> > static void *my_memcpy_resolver(void) { return &my_memcpy_impl; } // Now
> my_memcpy ifunc has defined resolver
> >
> > void CpuSpecific(void); // CpuSpecific is a Function declaration
> > void Foo(void) { CpuSpecific(); } // Foo calls a function declaration
> > __attribute__((cpu_specific(generic))) void CpuSpecific(void) {
> puts("generic"); } // We still don't know whether cpu_dispatch will be in
> this TU or not. Do we upgrade CpuSpecific from a function declaration to a
> definition? to an ifunc? Bind directly to this body?
> > __attribute__((cpu_dispatch(generic))) void CpuSpecific(void); // Now we
> know that we have to upgrade it to an ifunc, and how the resolver should
> look. But it's already a Function (declaration).
>
> All the above //need// to work in non-incremental compilation, and the only
> existing way for them to work right now is RAUW or a module finalization
> step. If the CFE code owner(s) find that offensive but proposes no
> alternative, what is one to do? There are 3 possible states here:
>
> 1. Remain broken
As mentioned above, not necessarily always broken, my downstream works with
your examples of how its broken. Breaking this is IMO, not acceptable.
> 2. Solve using RAUW/finalization
This is unacceptable to the Clang CFE maintainer, so I believe this is a
non-starter.
> 3. Solve using yet-unproposed non-RAUW solution (which solves all of the
> above use-cases simultaneously because they're essentially the same)
My preference is clearly this one. If we had a way to create the ifunc in the
CFE with NO resolver (that is, a forward-declaration of an ifunc!) that could
be emitted as-if it were a normal function call (which is what you suggested to
happen in the CFE, so this is just suggesting it later), it solves my problem.
As far as I can tell, this is what my downstream is doing (except the key-off
here is whether the resolver is not defined, not null). Can we discuss an
actual solution as a part of this discussion?
> It is better to treat (2) as a way out of (1) which doesn't increase the cost
> of (3), than to just stay at (1) until (3) happens. As it currently stands,
> the somewhat similar issue of calling a declared-but-undefined function in
> the REPL is currently embodied as a JIT session error, with failure to
> materialize symbols. Perhaps a valid solution for the work-in-progress
> aliases and ifuncs is to transform them into declarations in the JIT module
> until they have definitions for their aliasees/resolvers. But we won't know
> without more context. I think it might be more effective that I'll write a
> patch up which does use RAUW together with a test for the breakage we
> discussed and we'll continue the discussion with the CFE code owner(s) there.
>
>>> The LLVM bitcode verifier does not consider GlobalAliases which have either
>>> a null or an undefined aliasee to be valid. The same should have held for
>>> GlobalIFuncs and their resolvers since their inception, and the fact that
>>> it were not so is an oversight.
>>
>> Citation Needed here. Is there a design document that specifies this, or is
>> this your opinion? If its the latter, the implementation was the
>> documentation, so this is still a breaking change.
>
> I'm assuming that you're talking about GlobalIFuncs, since for GlobalAliases
> you'll see both constraints encoded in the `Verifier::visitGlobalAlias` and
> `Verifier::visitAliaseeSubExpr` functions.
> As for GlobalIFunc, I have no design document for the LLVM-IR-level
> representation of the feature, but:
>
> 1. @MaskRay has provided reference for the object-format constraint itself
> (`Requirement (a): Resolver must be defined in the same translation unit as
> the implementations.`). Bitcode Modules correspond to TUs - so it stands to
> reason that the same restriction apply to them, unless behavior is
> implemented to bridge the gap.
> 2. I've demonstrated that compiling a bitcode module containing an ifunc with
> an undefined resolver emits broken code for all usages of the
> ifunc-with-no-resolver, where I can probably massage that into a crash with a
> small bit of additional work (since it ends up calling the resolver rather
> than calling the return value of the resolver). This applies to current
> use-cases of cpu_specific in translation units that don't contain the
> corresponding cpu_dispatch, which I understand is part of the documented
> usage model of cpu_specific/cpu_dispatch. So it's already subtly broken, and
> in reference to (1), this means that behavior to bridge the gap between what
> the IR ostensibly allows and what the object format allows is **not**
> implemented.
> 3. CFE already diagnoses this issue for plain ifuncs and aliases.
So its important to note here that a 'language level' ifunc is not a perfect
match to the IR-level ifunc, which is not a perfect match to an object-file
ifunc. These are 3 different features that map closely, but not exactly.
> 4. I've drawn significant parallels between aliases and ifuncs at the object
> file level, and extrapolated the reasoning for why aliases have to point at
> defined objects to the reasoning why ifuncs have to point at defined objects.
>
> So, when I say "should have held" I am indeed expressing my opinion. But I do
> believe that I've backed it up substantially, and that from a design
> perspective adding that restriction to the IR really is the correct decision.
I appreciate the explanation.
> Calling the implementation the documentation doesn't ring right with me - is
> the broken behavior I referred to in (2) a compatibility constraint now? Are
> the bizarre disallowed-by-the-specification undefined STT_GNU_IFUNC symbols
> emitted by cpu_specific-without-cpu_dispatch a compatibility constraint now?
In absence of a spec, the implement-is-the-spec just by nature. As I
mentioned, my downstream DOES have this work, so this is a compatibility
constraint (and we as a project tend to at least try to work with downstreams
by not breaking them this aggressively). That said, I am, and have been willing
to work with you to make the reasonable changes here. I'm trying to get us to
a solution that doesn't break my users and allows you to recommit your patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112349/new/
https://reviews.llvm.org/D112349
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
