{"thread":{"id":"51669","subject":"[PATCH] grep: under --debug, show whether PCRE JIT is enabled","startedAt":"2019-08-18T20:24:56Z","lastAt":"2019-08-27T19:43:16Z","messageCount":8,"participants":["Beat Bolli","Junio C Hamano","Carlo Arenas","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380677","messageId":"20190818201727.31505-1-dev+git@drbeat.li","threadId":"51669","inReplyTo":null,"subject":"[PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2019-08-18T20:17:27Z","receivedAt":"2019-08-18T20:24:56Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"This information is useful and not visible anywhere else, so show it.\n\nSigned-off-by: Beat Bolli \nSuggested-by: Johannes Schindelin \n\n---\nThis applies on top of 'ab/pcre-jit-fixes', currently in pu.\n\n grep.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex 9bc589720b..96272b3cfc 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -433,6 +433,8 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \n #ifdef GIT_PCRE1_USE_JIT\n \tpcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);\n+\tif (opt->debug)\n+\t\tfprintf(stderr, \"pcre1_jit_on=%d\\n\", p->pcre1_jit_on);\n #endif\n }\n \n@@ -534,6 +536,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t}\n \n \tpcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);\n+\tif (opt->debug)\n+\t\tfprintf(stderr, \"pcre2_jit_on=%d\\n\", p->pcre2_jit_on);\n \tif (p->pcre2_jit_on) {\n \t\tjitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);\n \t\tif (jitret)\n@@ -559,6 +563,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tBUG(\"pcre2_pattern_info() failed: %d\", patinforet);\n \t\tif (jitsizearg == 0) {\n \t\t\tp->pcre2_jit_on = 0;\n+\t\t\tif (opt->debug)\n+\t\t\t\tfprintf(stderr, \"pcre2_jit_on=%d: (*NO_JIT) in regex\\n\",\n+\t\t\t\t\tp->pcre2_jit_on);\n \t\t\treturn;\n \t\t}\n \t}\n-- \n2.21.0.1020.gf2820cf01a\n\n"},{"id":"380744","messageId":"xmqqk1b8x0ac.fsf@gitster-ct.c.googlers.com","threadId":"51669","inReplyTo":"20190818201727.31505-1-dev+git@drbeat.li","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-19T22:23:07Z","receivedAt":"2019-08-19T22:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Beat Bolli writes:\n\n> This information is useful and not visible anywhere else, so show it.\n>\n> Signed-off-by: Beat Bolli \n> Suggested-by: Johannes Schindelin \n>\n> ---\n> This applies on top of 'ab/pcre-jit-fixes', currently in pu.\n\nThanks. \n\nWe saw a few people exchanging patches on the list and discussing\ntopics aroud PCRE and especially JIT, but the discussion petered out\nduring the prerelease freeze. I'd like to see the topics solidify\nearly in this cycle.\n\nIIRC, some key points/issues addressed by various patches we saw\nduring the last cycle were:\n\n - A binary may be prepared to be JIT capable, but on a particular\n system (e.g. SELinux) JIT may not work. Should we write off such\n a configuration as \"broken\"? Should we just fall back on non-JIT?\n Should we fall back with loud warning?\n\n - JIT and non-JIT codepath may validate UTF-8 differently without\n care, but we should make sure JIT codepath behave identically to\n non JIT (only faster).\n\n - We should not be validating strict UTF-8 when we do not even know\n if the payload is UTF-8. What mechanism, if any, do we have to\n let us say \"this must be UTF-8 or otherwise it is an error\" with\n confidence? Should we error out in the middle of \"git log\"\n session upon seeing a binary haystack while looking for UTF-8\n needle (I think not)?\n\nThere may be others I am missing.\n\nIs ab/pcre-jit-fixes a good base to collectively work on to finish\nthe topics floated around PCRE during the last cycle?\n\nI'll queue this debugging aid on top in the meantime. Thanks.\n\n"},{"id":"381068","messageId":"CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@mail.gmail.com","threadId":"51669","inReplyTo":"xmqqk1b8x0ac.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-24T12:57:52Z","receivedAt":"2019-08-24T12:58:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano wrote:\n>\n> There may be others I am missing.\n\nshould we still support PCRE1? I think in this case the problem is\ncompounded by the fact that unless we do something like [1], the real\nfix for those UTF-8 validation issues will require a yet unreleased\nversion of PCRE2 and will never be available for PCRE1, making the\nuser experience suboptimal.\n\nand explained in [1] there was a series to cleanup (both for\nmaintainability and to mitigate regressions) the PCRE1 code that is\nyet to be formally reviewed in [2]\n\nthere is also the question of if we should provide knobs so users can\n\"tune\" their pcre library to workaround some of the quirks or if we\nshould do more work ourselves to handle those quirks and improve the\nerror reporting.\n\none example of that is as you pointed out JIT, but also applies to\nother things like PCRE's stack size, or depending on our solution for\nPCRE1, accepting the risk (which already exist anyway) to accept\nproblems with matching because of corrupted UTF-8 in the haystack\n\n> Is ab/pcre-jit-fixes a good base to collectively work on to finish\n> the topics floated around PCRE during the last cycle?\n\nV3 of that (which was never sent) might be better IMHO, I had to also\nadmit I was surprised to see the whole no-kwset series this depended\non being dropped but would seem it was just partially merged with\npcre-jit-fixes (which is missing the patches that address the UTF-8\nissues with PCRE2's unreleased flag and that should be part of that\nV3)\n\nit might be worth also rebasing pcre2-chartables-leakfix on top of\nthis to avoid conflicts, eventhough I had to admit that I was\nexpanding on integrating [3], to avoid having to squash a fix into\nRené's patch (as he suggested) and that would be part of a reroll from\nthat series.\n\nCarlo\n\n[1] https://public-inbox.org/git/CAPUEspgStVxL=0SoAg82vxRMRGLSEKdHrT-xq6nCW1sNq7nLsw@mail.gmail.com/\n[2] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/\n[3] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/\n"},{"id":"381178","messageId":"nycvar.QRO.7.76.6.1908261626450.46@tvgsbejvaqbjf.bet","threadId":"51669","inReplyTo":"CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@mail.gmail.com","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-26T14:28:49Z","receivedAt":"2019-08-26T14:29:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Sat, 24 Aug 2019, Carlo Arenas wrote:\n\n> On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano wrote:\n> >\n> > There may be others I am missing.\n>\n> should we still support PCRE1?\n\nWhile Git for Windows has no problem to just drop PCRE1 support, I would\nlike to take a longer road in Git. Like, deprecate it first (I don't\nknow how to do that effectively, though, as packagers usually ignore\ncompile warnings, maybe we need to add a Makefile knob\nYES_I_WANT_TO_BUILD_WITH_PCRE1_INSTEAD_OF_PCRE2 and\nYES_I_WILL_STATE_MY_REASONS_ON_THE_GIT_MAILING_LIST or something like\nthat).\n\nCiao,\nDscho\n"},{"id":"381181","messageId":"CAPUEspivEfc82ZM=VaUA_sYhQbz+GtoOODVZK2i9LcCkmHq=Sg@mail.gmail.com","threadId":"51669","inReplyTo":"nycvar.QRO.7.76.6.1908261626450.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-26T14:42:56Z","receivedAt":"2019-08-26T14:43:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 26, 2019 at 7:29 AM Johannes Schindelin\n wrote:\n>\n> On Sat, 24 Aug 2019, Carlo Arenas wrote:\n>\n> > On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano wrote:\n> > >\n> > > There may be others I am missing.\n> >\n> > should we still support PCRE1?\n>\n> While Git for Windows has no problem to just drop PCRE1 support,\n\nFWIW I don't want to drop PCRE1 support, I was not advocating for it,\nbut in the contrary\ntrying to find a way to keep it working as best as possible until we\nreally can't.\n\n> I would\n> like to take a longer road in Git. Like, deprecate it first (I don't\n> know how to do that effectively, though, as packagers usually ignore\n> compile warnings, maybe we need to add a Makefile knob\n> YES_I_WANT_TO_BUILD_WITH_PCRE1_INSTEAD_OF_PCRE2 and\n> YES_I_WILL_STATE_MY_REASONS_ON_THE_GIT_MAILING_LIST or something like\n> that).\n\nFWIW e6c531b808 (Makefile: make USE_LIBPCRE=YesPlease mean v2, not v1,\n2018-03-11)\nput a \"deprecattion\" warning in the Makefile by Ævar, but last time\nthis was discussed[1] Junio\nmade an IMHO sound argument for why that should be removed instead but\nab/pcre-jit-fixes and UTF-8 validation are likely to make that more\ndifficult (even if it is a mostly self inflicted wound AFAIK)\n\nCarlo\n\n[1] https://public-inbox.org/git/xmqqlg4xkh28.fsf@gitster-ct.c.googlers.com/\n"},{"id":"381198","messageId":"xmqqftlnkj98.fsf@gitster-ct.c.googlers.com","threadId":"51669","inReplyTo":"CAPUEspivEfc82ZM=VaUA_sYhQbz+GtoOODVZK2i9LcCkmHq=Sg@mail.gmail.com","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T16:02:11Z","receivedAt":"2019-08-26T16:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> ... but\n> ab/pcre-jit-fixes and UTF-8 validation are likely to make that more\n> difficult (even if it is a mostly self inflicted wound AFAIK)\n\nHmm, in what way? Do you mean that we'd be invested even more in\npcre1 in an effort to keep supporting, that the sunk cost would\ndissuade us from deprecating the support even more, or something?\n\n"},{"id":"381207","messageId":"CAPUEspj4BJLjXorUXMiZnFtNcmhym_2QL5GUqeaGaCoxk=zjtw@mail.gmail.com","threadId":"51669","inReplyTo":"xmqqftlnkj98.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-26T16:36:38Z","receivedAt":"2019-08-26T16:36:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 26, 2019 at 9:02 AM Junio C Hamano wrote:\n>\n> Carlo Arenas writes:\n>\n> > ... but\n> > ab/pcre-jit-fixes and UTF-8 validation are likely to make that more\n> > difficult (even if it is a mostly self inflicted wound AFAIK)\n>\n> Hmm, in what way? Do you mean that we'd be invested even more in\n> pcre1 in an effort to keep supporting, that the sunk cost would\n> dissuade us from deprecating the support even more, or something?\n\non the contrary, PCRE1 works fine but our recent changes make it worst\nunnecessarily (IMHO)\n\n for example 685668faaa (grep: stop using a custom JIT stack with PCRE\nv1, 2019-07-26) adds 2 regressions as discussed in [1]\n\n* git grep -P will now throw an error if there are non UTF-8 documents\nin the haystack (even if JIT is available)\n* git grep -P '^([/](?!/)|[^/])*~/.*' will now fail with a cryptic\nPCRE error instead of succeeding (but at least will be consistent and\nshow the same error with PCRE2)\n\nCarlo\n\n[1] https://public-inbox.org/git/CAPUEspgStVxL=0SoAg82vxRMRGLSEKdHrT-xq6nCW1sNq7nLsw@mail.gmail.com/\n"},{"id":"381388","messageId":"xmqqzhju2y4b.fsf@gitster-ct.c.googlers.com","threadId":"51669","inReplyTo":"CAPUEspj4BJLjXorUXMiZnFtNcmhym_2QL5GUqeaGaCoxk=zjtw@mail.gmail.com","subject":"Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-27T19:43:00Z","receivedAt":"2019-08-27T19:43:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n>> > ... but\n>> > ab/pcre-jit-fixes and UTF-8 validation are likely to make that more\n>> > difficult (even if it is a mostly self inflicted wound AFAIK)\n>> ...\n> for example 685668faaa (grep: stop using a custom JIT stack with PCRE\n> v1, 2019-07-26) adds 2 regressions as discussed in [1]\n>\n> * git grep -P will now throw an error if there are non UTF-8 documents\n> in the haystack (even if JIT is available)\n> * git grep -P '^([/](?!/)|[^/])*~/.*' will now fail with a cryptic\n> PCRE error instead of succeeding (but at least will be consistent and\n> show the same error with PCRE2)\n\nThanks. This was exactly the kind of thing I wanted to hear when I\nasked if ab/pcre-jit-fixes is a good base to further build on.\n\nAs far as I am concerned, ab/pcre-jit-fixes and anything pcre that\nare still in flight are fair game for a reboot during this cycle (I\nhope I did not merge any of them to 'next' by mistake yet). If\nthere are bad apples queued on 'pu', let's make sure we eject them\nand rethink the way to address the issue(s) they wanted to address.\n\n"}]}