[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

dexonsmith added a comment. Herald added a project: All. In D112349#3330918 , @pirama wrote: > Unrelated to missing resolver definition, this change doesn't accommodate > resolvers that take parameters. (Curiously, this verification only fails > with T

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. I now realize that the type check isn't correct for the platforms which pass arguments to the resolver. Unfortunate that the glibc wiki doesn't mention this (as far as I can tell)... I thought that the bitcast-to-"expected"-type should shield from that error, but may

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

pirama added subscribers: pirama, srhines, kongyi. pirama added a comment. Unrelated to missing resolver definition, this change doesn't accommodate resolvers that take parameters. (Curiously, this verification only fails with ThinLTO). // with -flto=full or without -flto=thin, below command

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3116726 , @ibookstein wrote: > Who is the Clang CFE maintainer that we need to involve? Richard Smith. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ htt

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. Who is the Clang CFE maintainer that we need to involve? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 ___ cfe-commits mailing lis

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In 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

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. > 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,

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3112315 , @v.g.vassilev wrote: > In D112349#3111927 , @erichkeane > wrote: > >> In D112349#3111873 , @ibookstein >> wrote: >> >>>

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. I feel like we're getting lost in the weeds here. At the time a bitcode module is finalized, it is supposed to be in a valid state. The LLVM bitcode verifier does not consider GlobalAliases which have either a null or an undefined aliasee to be valid. The same shoul

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

v.g.vassilev added a comment. In D112349#3111927 , @erichkeane wrote: > In D112349#3111873 , @ibookstein > wrote: > >> And how is Cling expecting CFE to deal with partial knowledge situations at >> the implemen

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3111873 , @ibookstein wrote: > And how is Cling expecting CFE to deal with partial knowledge situations at > the implementation level? To deal with exactly the non-local cases that the > current violations address

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. And how is Cling expecting CFE to deal with partial knowledge situations at the implementation level? To deal with exactly the non-local cases that the current violations address? If there's no prescriptive way forward to dealing with these cases (so they're tech deb

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3109994 , @ibookstein wrote: > I see. What is the guiding principle there, though? Generating correct IR "up > front" / "the first time" rather than "fixing it up as you go via > manipulations"? (could you give a

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

MaskRay added a comment. Sorry for being late to the party. Itay's explanation is correct to me. I don't know much about multiversioned functions. My reply below if for ifunc. - On ELF, an alias is just a symbol sharing st_shndx/st_value with another symbol. It is not by name. The target symbol

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. I see. What is the guiding principle there, though? Generating correct IR "up front" / "the first time" rather than "fixing it up as you go via manipulations"? (could you give a link?) I can see the engineering consideration in not letting IR manipulations creep into

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. >> If the result wasn't null (call it F), use GI->takeName(F); >> F->replaceAllUsesWith(GI); When I wrote the multiversioning support in the first place, I was told that the above was unacceptable and the CFE doesn't wish to do that anymore. There ARE some examples

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

nextsilicon-itay-bookstein added a comment. That feature already exists - use a plain old function declaration :) My mental model for this is like this: memcpy one of the is the most widely popular APIs commonly implemented as an ifunc. In clients of this API, it's just a plain old function decl

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3109211 , @nextsilicon-itay-bookstein wrote: >> I don't know much about the ELF format... but this works today? We can >> define a resolver in a different TU and it WORKS thanks to the linker? So >> there is per

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

nextsilicon-itay-bookstein added a comment. > I don't know much about the ELF format... but this works today? We can > define a resolver in a different TU and it WORKS thanks to the linker? So > there is perhaps something? The ifunc symbol that is emitted in the TU with the undefined resolver

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. Hmm... we've never had any problems with cpu-dispatch/specific on this before? An example just like that works in all my internal tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.or

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

nextsilicon-itay-bookstein added a comment. It sort-of-works only because you define the ifunc in both translation units (with the same name). But looks like it behaves incorrectly for references to the ifunc in the translation unit where the resolver is only declared, not defined: > cat exa

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3109065 , @nextsilicon-itay-bookstein wrote: > I'm referring you again to the start of my explanation (the first three > paragraphs); the object file format (ELF) literally cannot express the > semantics you're as

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

nextsilicon-itay-bookstein added a comment. I'm referring you again to the start of my explanation (the first three paragraphs); the object file format (ELF) literally cannot express the semantics you're asking for. You're asking for it to support a symbol in a special kind of undefined state:

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. To continue my point... an ifunc/resolver is just like a function, in that a non-defined declaration is completely valid, since it refers to a definition in a separate TU. It makes sense to me that a resolver could do the same. Actually... I question the diagnostic

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3107460 , @ibookstein wrote: > I'll first explain my thought process about the representation of aliases and > ifuncs in the IR, and why I think both aliasees and resolvers must always be > defined; I hope I'm not

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. I'll first explain my thought process about the representation of aliases and ifuncs in the IR, and why I think both aliasees and resolvers must always be defined; I hope I'm not completely off track and would love it if @MaskRay could weigh in as to whether I make s

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. To 'unbreak' us for now, I've committed the suggested change to remove the definition check here: 09233412edae388a7bfa349cf792dba5aced057f Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. In D112349#3104479 , @ibookstein wrote: > Hmm. When I try to compile an object file where the resolver is a > declaration, both clang-13, clang-14, and gcc-9.3 complain that the ifunc > must point to a defined function: >

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. Note that (as the examples demonstrate) clang self-verifies and checks among other things that ifuncs that it emits point to definitions; this happens in `CodeGenModule::checkAliases()`. I haven't read the cpu_specific/cpu_dispatch-related code in CodeGenModule yet,

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. Hmm. When I try to compile an object file where the resolver is a declaration, both clang-13, clang-14, and gcc-9.3 complain that the ifunc must point to a defined function: void *foo_resolver(); void foo(void) __attribute__((ifunc("foo_resolver"))); clang (13 a

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. I note now that asserts build fails for it: https://godbolt.org/z/r738hGoKf Should this be reverted? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 __

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

erichkeane added a comment. So I've noticed in my downstream that this fires in the cpu-dispatch.c codegen test, though it doesn't seem to catch it here? I'm not sure how this happens, but from your description, it SEEMS like this case https://godbolt.org/z/nejWhbsxa should cause this error.

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

This revision was automatically updated to reflect the committed changes. Closed by commit rG848812a55e53: [Verifier] Add verification logic for GlobalIFuncs (authored by ibookstein, committed by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. Could you commit this on my behalf? :) 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@list

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 __

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein updated this revision to Diff 381773. ibookstein added a comment. Change BitcodeReader to transparently fix up the type rather than returning an error because older formats use wrong type. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D11

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein updated this revision to Diff 381760. ibookstein edited the summary of this revision. ibookstein added a comment. Herald added a subscriber: nemanjai. Added check to BitcodeReader, fixed clang tests, hoisted logic to shared static function on GlobalIFunc. Repository: rG LLVM Github

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein added a comment. Because this commit changes an existing binary bitcode file, and because that file specifically tests backwards compatibility, does that mean I need to avoid changing it and instead add a backwards compatibility fix to the BitcodeReader? (Something like always bitcas

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein updated this revision to Diff 381754. ibookstein added a comment. Now using arcanist because commit includes change to binary file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 Files: clan

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein updated this revision to Diff 381751. ibookstein added a comment. rebase CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112349/new/ https://reviews.llvm.org/D112349 Files: clang/lib/CodeGen/CodeGenModule.cpp llvm/lib/IR/Globals.cpp llvm/lib/IR/Verifier.cpp llvm/test/A

[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

ibookstein updated this revision to Diff 381735. ibookstein set the repository for this revision to rG LLVM Github Monorepo. ibookstein added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed type verification to look at the resolver operand rather than the