Pete Cooper via llvm-dev
2015-Aug-31 18:14 UTC
[llvm-dev] Should personality functions actually be functions
(Moving to LLVM-dev). I hope thats ok to just move the conversation over from commits. To summarise, selection DAG crashes if personality functions are not actually functions. This conversation is to try work out if that is a restriction we want to document in LangRef, or (as David B suggests), we should allow opaque values to be personality functions. I don’t actually know EH code at all, so I don’t know what the right answer is.> On Aug 31, 2015, at 11:06 AM, Philip Reames <listmail at philipreames.com> wrote: > > I think this question should be moved to llvm-dev.Yep, makes sense.> David's point is completely reasonable, but I think we generally assume personality functions are functions today. I'd be tempted to modify the verifier as Pete suggests, but we should get broader input before actually doing so. Given how vague the LangRef is today, restricting personality functions to be direct calls could be construed as a LangRef change.Yeah, this is a little vague. Considering the code in SelectionDAGBuilder will always crash if we have a non-Function*, I believe that means we could never have actually codegen-ed any of the code I had to fix in test cases. ...> > Philip > > On 08/29/2015 08:56 AM, David Blaikie wrote: >> Knowing nothing about any of this stuff, I do find this a bit surprising - usually values are opaque to their consumers: they're just a value.So i actually know nothing about this either. When i originally found the crash, personality functions were still on the LandingPadInst itself. I agree that typically its fine to be able to use opaque values. Unless we actually have to introspect the personality Function*, eg, for calling conventions, then I think its fine to allow any value. In fact, it looks like classifyEHPersonality was written to support this. Pete>> >> (especially with pointer bitcasts (which will go away, eventually) - if I have a global variable I've put some executable bytes in and I cast a pointer to that global variable to a pointer to a function, I would expect that should be interchangeable...) >> >> On Fri, Aug 28, 2015 at 5:33 PM, Pete Cooper via llvm-commits < <mailto:llvm-commits at lists.llvm.org>llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote: >> Hi Philip >> >> While reducing a test case with personality functions, I crashed selection DAG. The reason being that my personality function was no longer a function. The relevant code is >> >> MF->getMMI().addPersonality(MBB, cast<Function>(LPadInst->getParent() >> ->getParent() >> ->getPersonalityFn() >> ->stripPointerCasts())); >> >> We get different behavior in this code, which is able to handle non-function personality functions. >> >> EHPersonality llvm::classifyEHPersonality(const Value *Pers) { >> const Function *F = dyn_cast<Function>(Pers->stripPointerCasts()); >> if (!F) >> return EHPersonality::Unknown; >> >> And finally, LangRef itself implies that its a function as it says that we should 'specify what function to use for exception handling’. >> >> So i’m not sure exactly what behavior we want. >> >> However, assuming we do want personality functions to always actually be functions (which was what i originally thought we wanted), here’s a patch which teaches the verifier this. >> >> Comments welcome, including the possibility that this is completely wrong and that we do want to support other values as personalities. >> >> Cheers >> Pete >> >> >> >> _______________________________________________ >> llvm-commits mailing list >> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQMDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=osq1cWQ7EmXsSMMetgDarRODQ97erXe70D8IoZAGoTM&s=eMTJOIwzYQF_n2Vl-3YdQp5usXIGF5Xfo1FT82cuyt0&e=> >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150831/c17d8e49/attachment.html>
David Blaikie via llvm-dev
2015-Aug-31 18:19 UTC
[llvm-dev] Should personality functions actually be functions
+Majnemer, since he's been mucking about with this stuff recently On Mon, Aug 31, 2015 at 11:14 AM, Pete Cooper <peter_cooper at apple.com> wrote:> (Moving to LLVM-dev). I hope thats ok to just move the conversation over > from commits. > > To summarise, selection DAG crashes if personality functions are not > actually functions. This conversation is to try work out if that is a > restriction we want to document in LangRef, or (as David B suggests), we > should allow opaque values to be personality functions. I don’t actually > know EH code at all, so I don’t know what the right answer is. > > On Aug 31, 2015, at 11:06 AM, Philip Reames <listmail at philipreames.com> > wrote: > > I think this question should be moved to llvm-dev. > > Yep, makes sense. > > David's point is completely reasonable, but I think we generally assume > personality functions are functions today. I'd be tempted to modify the > verifier as Pete suggests, but we should get broader input before actually > doing so. Given how vague the LangRef is today, restricting personality > functions to be direct calls could be construed as a LangRef change. > > Yeah, this is a little vague. Considering the code in SelectionDAGBuilder > will always crash if we have a non-Function*, I believe that means we could > never have actually codegen-ed any of the code I had to fix in test cases. > > ... > > > Philip > > On 08/29/2015 08:56 AM, David Blaikie wrote: > > Knowing nothing about any of this stuff, I do find this a bit surprising - > usually values are opaque to their consumers: they're just a value. > > So i actually know nothing about this either. When i originally found the > crash, personality functions were still on the LandingPadInst itself. > > I agree that typically its fine to be able to use opaque values. Unless > we actually have to introspect the personality Function*, eg, for calling > conventions, then I think its fine to allow any value. In fact, it looks > like classifyEHPersonality was written to support this. > > Pete > > > (especially with pointer bitcasts (which will go away, eventually) - if I > have a global variable I've put some executable bytes in and I cast a > pointer to that global variable to a pointer to a function, I would expect > that should be interchangeable...) > > On Fri, Aug 28, 2015 at 5:33 PM, Pete Cooper via llvm-commits < > <llvm-commits at lists.llvm.org>llvm-commits at lists.llvm.org> wrote: > >> Hi Philip >> >> While reducing a test case with personality functions, I crashed >> selection DAG. The reason being that my personality function was no longer >> a function. The relevant code is >> >> MF->getMMI().addPersonality(MBB, cast<Function>(LPadInst->getParent() >> ->getParent() >> ->getPersonalityFn() >> >> ->stripPointerCasts())); >> >> We get different behavior in this code, which is able to handle >> non-function personality functions. >> >> EHPersonality llvm::classifyEHPersonality(const Value *Pers) { >> const Function *F = dyn_cast<Function>(Pers->stripPointerCasts()); >> if (!F) >> return EHPersonality::Unknown; >> >> And finally, LangRef itself implies that its a function as it says that >> we should 'specify what function to use for exception handling’. >> >> So i’m not sure exactly what behavior we want. >> >> However, assuming we do want personality functions to always actually be >> functions (which was what i originally thought we wanted), here’s a patch >> which teaches the verifier this. >> >> Comments welcome, including the possibility that this is completely wrong >> and that we do want to support other values as personalities. >> >> Cheers >> Pete >> >> >> >> _______________________________________________ >> llvm-commits mailing list >> llvm-commits at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits >> <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQMDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=osq1cWQ7EmXsSMMetgDarRODQ97erXe70D8IoZAGoTM&s=eMTJOIwzYQF_n2Vl-3YdQp5usXIGF5Xfo1FT82cuyt0&e=> >> >> > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150831/a0d891d1/attachment.html>
Reid Kleckner via llvm-dev
2015-Aug-31 18:54 UTC
[llvm-dev] Should personality functions actually be functions
I think the easiest path forward is to keep getPersonalityFn returning a Constant and make this a dyn_cast. I'm halfway done with fixing it. On Mon, Aug 31, 2015 at 11:19 AM, David Blaikie via llvm-dev < llvm-dev at lists.llvm.org> wrote:> +Majnemer, since he's been mucking about with this stuff recently > > On Mon, Aug 31, 2015 at 11:14 AM, Pete Cooper <peter_cooper at apple.com> > wrote: > >> (Moving to LLVM-dev). I hope thats ok to just move the conversation over >> from commits. >> >> To summarise, selection DAG crashes if personality functions are not >> actually functions. This conversation is to try work out if that is a >> restriction we want to document in LangRef, or (as David B suggests), we >> should allow opaque values to be personality functions. I don’t actually >> know EH code at all, so I don’t know what the right answer is. >> >> On Aug 31, 2015, at 11:06 AM, Philip Reames <listmail at philipreames.com> >> wrote: >> >> I think this question should be moved to llvm-dev. >> >> Yep, makes sense. >> >> David's point is completely reasonable, but I think we generally assume >> personality functions are functions today. I'd be tempted to modify the >> verifier as Pete suggests, but we should get broader input before actually >> doing so. Given how vague the LangRef is today, restricting personality >> functions to be direct calls could be construed as a LangRef change. >> >> Yeah, this is a little vague. Considering the code in >> SelectionDAGBuilder will always crash if we have a non-Function*, I believe >> that means we could never have actually codegen-ed any of the code I had to >> fix in test cases. >> >> ... >> >> >> Philip >> >> On 08/29/2015 08:56 AM, David Blaikie wrote: >> >> Knowing nothing about any of this stuff, I do find this a bit surprising >> - usually values are opaque to their consumers: they're just a value. >> >> So i actually know nothing about this either. When i originally found >> the crash, personality functions were still on the LandingPadInst itself. >> >> I agree that typically its fine to be able to use opaque values. Unless >> we actually have to introspect the personality Function*, eg, for calling >> conventions, then I think its fine to allow any value. In fact, it looks >> like classifyEHPersonality was written to support this. >> >> Pete >> >> >> (especially with pointer bitcasts (which will go away, eventually) - if I >> have a global variable I've put some executable bytes in and I cast a >> pointer to that global variable to a pointer to a function, I would expect >> that should be interchangeable...) >> >> On Fri, Aug 28, 2015 at 5:33 PM, Pete Cooper via llvm-commits < >> <llvm-commits at lists.llvm.org>llvm-commits at lists.llvm.org> wrote: >> >>> Hi Philip >>> >>> While reducing a test case with personality functions, I crashed >>> selection DAG. The reason being that my personality function was no longer >>> a function. The relevant code is >>> >>> MF->getMMI().addPersonality(MBB, cast<Function>(LPadInst->getParent() >>> ->getParent() >>> >>> ->getPersonalityFn() >>> >>> ->stripPointerCasts())); >>> >>> We get different behavior in this code, which is able to handle >>> non-function personality functions. >>> >>> EHPersonality llvm::classifyEHPersonality(const Value *Pers) { >>> const Function *F = dyn_cast<Function>(Pers->stripPointerCasts()); >>> if (!F) >>> return EHPersonality::Unknown; >>> >>> And finally, LangRef itself implies that its a function as it says that >>> we should 'specify what function to use for exception handling’. >>> >>> So i’m not sure exactly what behavior we want. >>> >>> However, assuming we do want personality functions to always actually be >>> functions (which was what i originally thought we wanted), here’s a patch >>> which teaches the verifier this. >>> >>> Comments welcome, including the possibility that this is completely >>> wrong and that we do want to support other values as personalities. >>> >>> Cheers >>> Pete >>> >>> >>> >>> _______________________________________________ >>> llvm-commits mailing list >>> llvm-commits at lists.llvm.org >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits >>> <https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQMDaQ&c=eEvniauFctOgLOKGJOplqw&r=03tkj3107244TlY4t3_hEgkDY-UG6gKwwK0wOUS3qjM&m=osq1cWQ7EmXsSMMetgDarRODQ97erXe70D8IoZAGoTM&s=eMTJOIwzYQF_n2Vl-3YdQp5usXIGF5Xfo1FT82cuyt0&e=> >>> >>> >> >> >> > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150831/05742491/attachment.html>