zsrkmyn added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3002
false);
llvm::Constant *Resolver = GetOrCreateLLVMFunction(
MangledName + ".resolver", ResolverType, GlobalDecl{},
----------------
erichkeane wrote:
> zsrkmyn wrote:
> > erichkeane wrote:
> > > zsrkmyn wrote:
> > > > zsrkmyn wrote:
> > > > > erichkeane wrote:
> > > > > > zsrkmyn wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > This Resolver should have the same linkage as below.
> > > > > > > Actually, I wanted to set linkage here at the first time, but
> > > > > > > failed. When compiling code with cpu_specific but no
> > > > > > > cpu_dispatch, we cannot set it as LinkOnceODR or WeakODR. E.g.:
> > > > > > >
> > > > > > > ```
> > > > > > > $ cat specific_only.c
> > > > > > > __declspec(cpu_specific(pentium_iii))
> > > > > > > int foo(void) { return 0; }
> > > > > > > int usage() { return foo(); }
> > > > > > >
> > > > > > > $ clang -fdeclspec specific_only.c
> > > > > > >
> > > > > > > Global is external, but doesn't have external or weak linkage!
> > > > > > >
> > > > > > > i32 ()* ()* @foo.resolver
> > > > > > >
> > > > > > > fatal error: error in backend: Broken module found, compilation
> > > > > > > aborted!
> > > > > > > ```
> > > > > > >
> > > > > > > This is found by lit test test/CodeGen/attr-cpuspecific.c, in
> > > > > > > which 'SingleVersion()' doesn't have a cpu_dispatch declaration.
> > > > > > The crash message is complaining it isn't external/weak. However,
> > > > > > WeakODR should count, right? Can you look into it a bit more to
> > > > > > see what it thinks is broken?
> > > > > No, actually I've tried it earlier with the example I mentioned in my
> > > > > last comment, but WeakODR still makes compiler complaining. I think
> > > > > it's `foo.resolver` that cannot be declared with as
> > > > > WeakODR/LinkOnceODR without definition. But I'm really not familiar
> > > > > with these rules.
> > > > According to the `Verifier::visitGlobalValue()` in Verify.cpp, an
> > > > declaration can only be `ExternalLinkage` or `ExternalWeakLinkage`. So
> > > > I still believe we cannot set the resolver to
> > > > `LinkOnceODRLinkage/WeakODRLinkage` here, as they are declared but not
> > > > defined when we only have `cpu_specified` but no `cpu_dispatch` in a TU
> > > > as the example above.
> > > That doesn't seem right then. IF it allows ExternalWeakLinkage I'd
> > > expect WeakODR to work as well, since it is essentially the same thing.
> > I think we should have a double check. It is said "It is illegal for a
> > function declaration to have any linkage type other than `external` or
> > `extern_weak`" at the last line of section Linkage Type in the reference
> > manual [1]. I guess `weak_odr` is not designed for declaration purpose and
> > should be only used by definition.
> >
> > [1] https://llvm.org/docs/LangRef.html#linkage-types
> I had typed a reply, but apparently it didn't submit: Ah, nvm, I see now that
> external-weak is different from weak.
>
> I don't really get the linkages sufficiently to know what the right thing to
> do is then. If we DO have a definition, I'd say weak_odr so it can be
> merged, right? If we do NOT, could externally_available work?
No, I think it should be `external` instead of `available_externally`. The
later cannot used for declaration as well.
So, getting back to the example, **1)** if we have `cpu_dispatch` and
`cpu_specific` in same TU, it's okay to use `weak_odr` for `foo.resolver` as it
is defined when `emitCPUDispatchDefinition` and it can be merged. **2)** If we
only have `cpu_specific` in a TU and have a reference to the dispatched
function, `foo.resolver` will be referenced without definition, and `external`
is the proper linkage to make it work.
That's why I didn't set linkage type at this line.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67058/new/
https://reviews.llvm.org/D67058
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
