{"thread":{"id":"51731","subject":"[PATCH 0/2] PCRE1 cleanup","startedAt":"2019-08-25T18:22:30Z","lastAt":"2019-08-27T01:34:55Z","messageCount":6,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"381119","messageId":"20190825182223.76288-1-carenas@gmail.com","threadId":"51731","inReplyTo":null,"subject":"[PATCH 0/2] PCRE1 cleanup","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-25T18:22:21Z","receivedAt":"2019-08-25T18:22:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This is a reroll of [1] which is mostly equivalent to the original RFC\nbut rebased on top of ab/pcre-jit-fixes.\n\nThe first patch fixes an inconsistency from 685668faaa (grep: stop\nusing a custom JIT stack with PCRE v1, 2019-07-26) where NO_LIBPCRE1_JIT\nwill only affect versions of pcre1 >= 8.32, while support for JIT was added\nto pcre1 with version 8.20. Technically this is a change of behaviour as\noriginally it was not possible to use JIT with those older versions, but\nthe restriction was somehow arbitrary and caused by the use of JIT fast path. \n\nThe second patch is mainly refactoring and to make sure the solution from\n2fff1e196d (grep: fix NO_LIBPCRE1_JIT to fully disable JIT, 2017-11-12) is\nworking as expected.\n\nCarlo Marcelo Arenas Belón (2):\n grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1\n grep: refactor and simplify PCRE1 support\n\n Makefile | 9 ++-------\n grep.c | 16 ++++++++++------\n grep.h | 11 -----------\n 3 files changed, 12 insertions(+), 24 deletions(-)\n\n[1] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/\n\nbase-commit: c581e4a7499b9e1089847dbbc057afbef1ed861e\n-- \n2.23.0\n"},{"id":"381120","messageId":"20190825182223.76288-3-carenas@gmail.com","threadId":"51731","inReplyTo":"20190825182223.76288-1-carenas@gmail.com","subject":"[PATCH 2/2] grep: refactor and simplify PCRE1 support","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-25T18:22:23Z","receivedAt":"2019-08-25T18:22:33Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"The code used both a macro and a variable to keep track if JIT\nsupport was desired and relied on the fact that a non JIT\nenabled library will ignore a request for JIT compilation\n(as defined by the second parameter of the call to pcre_study)\n\nCleanup the multiple levels of macros used and call pcre_study\nwith the right parameter after JIT support has been confirmed\nand unless it was requested to be disabled with NO_LIBPCRE1_JIT\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 16 ++++++++++------\n grep.h | 9 ---------\n 2 files changed, 10 insertions(+), 15 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 76088784e3..05d31c2bcc 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -374,6 +374,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tconst char *error;\n \tint erroffset;\n \tint options = PCRE_MULTILINE;\n+\tint study_options = 0;\n \n \tif (opt->ignore_case) {\n \t\tif (!opt->ignore_locale && has_non_ascii(p->pattern))\n@@ -388,15 +389,18 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n \tif (!p->pcre1_regexp)\n \t\tcompile_regexp_failed(p, error);\n \n-\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, GIT_PCRE_STUDY_JIT_COMPILE, &error);\n-\tif (!p->pcre1_extra_info && error)\n-\t\tdie(\"%s\", error);\n-\n-#ifdef GIT_PCRE1_USE_JIT\n+#if defined(PCRE_CONFIG_JIT) && !defined(NO_LIBPCRE1_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+\n+\tif (p->pcre1_jit_on)\n+\t\tstudy_options = PCRE_STUDY_JIT_COMPILE;\n #endif\n+\n+\tp->pcre1_extra_info = pcre_study(p->pcre1_regexp, study_options, &error);\n+\tif (!p->pcre1_extra_info && error)\n+\t\tdie(\"%s\", error);\n }\n \n static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n@@ -425,7 +429,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n static void free_pcre1_regexp(struct grep_pat *p)\n {\n \tpcre_free(p->pcre1_regexp);\n-#ifdef GIT_PCRE1_USE_JIT\n+#ifdef PCRE_CONFIG_JIT\n \tif (p->pcre1_jit_on)\n \t\tpcre_free_study(p->pcre1_extra_info);\n \telse\ndiff --git a/grep.h b/grep.h\nindex 1a044c501e..ff620d784a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -3,15 +3,6 @@\n #include \"color.h\"\n #ifdef USE_LIBPCRE1\n #include \n-#ifndef NO_LIBPCRE1_JIT\n-#ifdef PCRE_CONFIG_JIT\n-#define GIT_PCRE1_USE_JIT\n-#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE\n-#endif\n-#endif\n-#ifndef GIT_PCRE_STUDY_JIT_COMPILE\n-#define GIT_PCRE_STUDY_JIT_COMPILE 0\n-#endif\n #else\n typedef int pcre;\n typedef int pcre_extra;\n-- \n2.23.0\n\n"},{"id":"381121","messageId":"20190825182223.76288-2-carenas@gmail.com","threadId":"51731","inReplyTo":"20190825182223.76288-1-carenas@gmail.com","subject":"[PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-25T18:22:22Z","receivedAt":"2019-08-25T18:22:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"e87de7cab4 (\"grep: un-break building with PCRE < 8.32\", 2017-05-25)\nadded a restriction for JIT support that is no longer needed after\npcre_jit_exec() calls were removed.\n\nReorganize the definitions in grep.h so that JIT support could be\ndetected early and NO_LIBPCRE1_JIT could be used reliably to enforce\nJIT doesn't get used.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 9 ++-------\n grep.h | 4 +---\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex f58bf14c7b..3f78ef942f 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -34,13 +34,8 @@ all::\n # library. Support for version 1 will likely be removed in some future\n # release of Git, as upstream has all but abandoned it.\n #\n-# When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1\n-# library is compiled without --enable-jit. We will auto-detect\n-# whether the version of the PCRE v1 library in use has JIT support at\n-# all, but we unfortunately can't auto-detect whether JIT support\n-# hasn't been compiled in in an otherwise JIT-supporting version. If\n-# you have link-time errors about a missing `pcre_jit_exec` define\n-# this, or recompile PCRE v1 with --enable-jit.\n+# When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if you want to\n+# disable JIT even if supported by your library.\n #\n # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are\n # in /foo/bar/include and /foo/bar/lib directories. Which version of\ndiff --git a/grep.h b/grep.h\nindex c0c71eb4a9..1a044c501e 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -3,14 +3,12 @@\n #include \"color.h\"\n #ifdef USE_LIBPCRE1\n #include \n-#ifdef PCRE_CONFIG_JIT\n-#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n #ifndef NO_LIBPCRE1_JIT\n+#ifdef PCRE_CONFIG_JIT\n #define GIT_PCRE1_USE_JIT\n #define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE\n #endif\n #endif\n-#endif\n #ifndef GIT_PCRE_STUDY_JIT_COMPILE\n #define GIT_PCRE_STUDY_JIT_COMPILE 0\n #endif\n-- \n2.23.0\n\n"},{"id":"381235","messageId":"xmqq36hnhi4n.fsf@gitster-ct.c.googlers.com","threadId":"51731","inReplyTo":"20190825182223.76288-2-carenas@gmail.com","subject":"Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T18:54:48Z","receivedAt":"2019-08-26T18:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> e87de7cab4 (\"grep: un-break building with PCRE < 8.32\", 2017-05-25)\n> added a restriction for JIT support that is no longer needed after\n> pcre_jit_exec() calls were removed.\n\nI was initially puzzled by this statement, until I realized that the\nremoval of pcre_jit_exec() happens in the topic still in flight that\nthis patch builds on top of, namely 685668fa (\"grep: stop using a\ncustom JIT stack with PCRE v1\", 2019-07-26).\n\nSo the logic is that because we do no longer call pcre_jit_exec()\nthat weren't available between 8.20 and 8.32, these slightly older\nversions can now do JIT just like the ones post 8.32?\n\nThanks. Queued.\n"},{"id":"381236","messageId":"xmqqy2zfg3f1.fsf@gitster-ct.c.googlers.com","threadId":"51731","inReplyTo":"20190825182223.76288-3-carenas@gmail.com","subject":"Re: [PATCH 2/2] grep: refactor and simplify PCRE1 support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T18:57:54Z","receivedAt":"2019-08-26T18:58:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> diff --git a/grep.h b/grep.h\n> index 1a044c501e..ff620d784a 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -3,15 +3,6 @@\n> #include \"color.h\"\n> #ifdef USE_LIBPCRE1\n> #include \n> -#ifndef NO_LIBPCRE1_JIT\n> -#ifdef PCRE_CONFIG_JIT\n> -#define GIT_PCRE1_USE_JIT\n> -#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE\n> -#endif\n> -#endif\n> -#ifndef GIT_PCRE_STUDY_JIT_COMPILE\n> -#define GIT_PCRE_STUDY_JIT_COMPILE 0\n> -#endif\n> #else\n> typedef int pcre;\n> typedef int pcre_extra;\n\nThat's a patch pleasant to read ;-)\n\nWill queue. Thansk.\n"},{"id":"381280","messageId":"CAPUEspjnwbC8L+9Ni_PN5kzROh3cqxEQRrhZHG_dhiR-K25OJA@mail.gmail.com","threadId":"51731","inReplyTo":"xmqq36hnhi4n.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-27T01:34:42Z","receivedAt":"2019-08-27T01:34:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 26, 2019 at 11:54 AM Junio C Hamano wrote:\n>\n> Carlo Marcelo Arenas Belón writes:\n>\n> > e87de7cab4 (\"grep: un-break building with PCRE < 8.32\", 2017-05-25)\n> > added a restriction for JIT support that is no longer needed after\n> > pcre_jit_exec() calls were removed.\n>\n> I was initially puzzled by this statement, until I realized that the\n> removal of pcre_jit_exec() happens in the topic still in flight that\n> this patch builds on top of, namely 685668fa (\"grep: stop using a\n> custom JIT stack with PCRE v1\", 2019-07-26).\n\nsorry about that, I thought I had mentioned it in the cover letter\n(since the hash is likely to change and so is not fit for a commit\nmessage) but it is not there either.\n\nhow could this be tracked more effectively?\n\n> So the logic is that because we do no longer call pcre_jit_exec()\n> that weren't available between 8.20 and 8.32, these slightly older\n> versions can now do JIT just like the ones post 8.32?\n\nexactly; but also because it is no longer using the JIT fast path\nwhich skipped UTF-8 validation, will need a way to disable that or\nrisk a regression as I mentioned in [1]\n\nwas planning in proposing a fix for PCRE1 based on [2] but wasn't sure\nif it could be part of this series, an independent one that is also\nbased on ab/pcre-jit-fixes, and like this one, is mostly a consequence\nof 685668fa (\"grep: stop using a custom JIT stack with PCRE v1\",\n2019-07-26) or something else, specially considering that Ævar\ndismissed it as a non issue in his commit message.\n\nCarlo\n\n[1] https://public-inbox.org/git/CAPUEspj4BJLjXorUXMiZnFtNcmhym_2QL5GUqeaGaCoxk=zjtw@mail.gmail.com/T/#m6acc8f68c398951457da469530bafa7e18811366\n[2] https://public-inbox.org/git/20190721183115.14985-1-carenas@gmail.com/\n"}]}