{"thread":{"id":"47182","subject":"[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","startedAt":"2017-11-12T17:07:16Z","lastAt":"2017-11-14T02:12:10Z","messageCount":5,"participants":["Charles Bailey","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332349","messageId":"20171112165938.8787-1-charles@hashpling.org","threadId":"47182","inReplyTo":null,"subject":"[PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2017-11-12T16:59:38Z","receivedAt":"2017-11-12T17:07:16Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey \n\nIf you have a pcre1 library which is compiled with JIT enabled then\nPCRE_STUDY_JIT_COMPILE will be defined whether or not the\nNO_LIBPCRE1_JIT configuration is set.\n\nThis means that we enable JIT functionality when calling pcre_study\neven if NO_LIBPCRE1_JIT has been explicitly set and we just use plain\npcre_exec later.\n\nFix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to\nPCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to\n0 otherwise, as before.\n---\n\nI was bisecting an issue with the PCRE support that was causing a test\nsuite failure on our Solaris builds and reached fbaceaac47 (\"grep: add\nsupport for the PCRE v1 JIT API\"). It appeared to be a misaligned memory\naccess somewhere inside the libpcre code. I tried disabling the use of\nJIT with NO_LIBPCRE1_JIT but it turned out that even with this set we\nwere still triggering the JIT code path in the call to pcre_study.\n\nYes, we probably should fix our PCRE1 library build on Solaris or move\nto PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from\ntriggering this crash.\n\n grep.c | 2 +-\n grep.h | 5 +++--\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex ce6a48e..d0b9b6c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -387,7 +387,7 @@ 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, PCRE_STUDY_JIT_COMPILE, &error);\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 \ndiff --git a/grep.h b/grep.h\nindex 52aecfa..399381c 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -7,11 +7,12 @@\n #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n #ifndef NO_LIBPCRE1_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 PCRE_STUDY_JIT_COMPILE\n-#define PCRE_STUDY_JIT_COMPILE 0\n+#ifndef GIT_PCRE_STUDY_JIT_COMPILE\n+#define GIT_PCRE_STUDY_JIT_COMPILE 0\n #endif\n #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20\n typedef int pcre_jit_stack;\n-- \n2.10.2\n\n"},{"id":"332362","messageId":"87tvxzxm0j.fsf@evledraar.booking.com","threadId":"47182","inReplyTo":"20171112165938.8787-1-charles@hashpling.org","subject":"Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-11-12T20:47:08Z","receivedAt":"2017-11-12T20:47:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 12 2017, Charles Bailey jotted:\n\n> From: Charles Bailey \n>\n> If you have a pcre1 library which is compiled with JIT enabled then\n> PCRE_STUDY_JIT_COMPILE will be defined whether or not the\n> NO_LIBPCRE1_JIT configuration is set.\n>\n> This means that we enable JIT functionality when calling pcre_study\n> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain\n> pcre_exec later.\n>\n> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to\n> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to\n> 0 otherwise, as before.\n> ---\n>\n> I was bisecting an issue with the PCRE support that was causing a test\n> suite failure on our Solaris builds and reached fbaceaac47 (\"grep: add\n> support for the PCRE v1 JIT API\"). It appeared to be a misaligned memory\n> access somewhere inside the libpcre code. I tried disabling the use of\n> JIT with NO_LIBPCRE1_JIT but it turned out that even with this set we\n> were still triggering the JIT code path in the call to pcre_study.\n>\n> Yes, we probably should fix our PCRE1 library build on Solaris or move\n> to PCRE2, but really NO_LIBPCRE1_JIT should have prevented us from\n> triggering this crash.\n>\n> grep.c | 2 +-\n> grep.h | 5 +++--\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index ce6a48e..d0b9b6c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -387,7 +387,7 @@ 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, PCRE_STUDY_JIT_COMPILE, &error);\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> diff --git a/grep.h b/grep.h\n> index 52aecfa..399381c 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -7,11 +7,12 @@\n> #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n> #ifndef NO_LIBPCRE1_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 PCRE_STUDY_JIT_COMPILE\n> -#define PCRE_STUDY_JIT_COMPILE 0\n> +#ifndef GIT_PCRE_STUDY_JIT_COMPILE\n> +#define GIT_PCRE_STUDY_JIT_COMPILE 0\n> #endif\n> #if PCRE_MAJOR <= 8 && PCRE_MINOR < 20\n> typedef int pcre_jit_stack;\n\n[CC-ing Junio]\n\nThanks a lot. This patch looks good to me.\n\nI could have sworn I was handling this already, but looking at this now\nI wasn't really.\n\nHowever, as a bit of extra info I *did* test this, and it works just\nfine for me, i.e. if I compile PCRE 8.32 now (as I did at the time)\n--without-jit it'll error with just USE_LIBPCRE=YesPlease as expected,\nbut add NO_LIBPCRE1_JIT=UnfortunatelyYes and it works just fine without\nyour patch.\n\nHowever, as your patch shows (and as I've independently verified)\nPCRE_STUDY_JIT_COMPILE will still be defined in that case, since PCRE\nwill be exposing the same headers. This is the logic error in my initial\npatch.\n\n*But* for some reason you still get away with that on Linux. I don't\nknow why, but I assume the compiler toolchain is more lax for some\nreason than on Solaris.\n\nAll of which is a roundabout way of saying that we should apply this\npatch, but that I still have no idea why this worked on Linux before ,\nbut it does.\n\nBut that we should take it anyway regardless of that since it'll *also*\nwork on Linux with your patch, and this logic makes some sense whereas\nthe other one clearly didn't and just worked by pure accident of some\ntoolchain semantics that I haven't figured out yet.\n"},{"id":"332388","messageId":"xmqqmv3qal78.fsf@gitster.mtv.corp.google.com","threadId":"47182","inReplyTo":"87tvxzxm0j.fsf@evledraar.booking.com","subject":"Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T03:53:15Z","receivedAt":"2017-11-13T03:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Sun, Nov 12 2017, Charles Bailey jotted:\n>\n>> From: Charles Bailey \n>>\n>> If you have a pcre1 library which is compiled with JIT enabled then\n>> PCRE_STUDY_JIT_COMPILE will be defined whether or not the\n>> NO_LIBPCRE1_JIT configuration is set.\n>>\n>> This means that we enable JIT functionality when calling pcre_study\n>> even if NO_LIBPCRE1_JIT has been explicitly set and we just use plain\n>> pcre_exec later.\n>>\n>> Fix this by using own macro (GIT_PCRE_STUDY_JIT_COMPILE) which we set to\n>> PCRE_STUDY_JIT_COMPILE only if NO_LIBPCRE1_JIT is not set and define to\n>> 0 otherwise, as before.\n>> ---\n>>\n>> I was bisecting an issue with the PCRE support that was causing a test\n>> ...\n>\n> [CC-ing Junio]\n>\n> Thanks a lot. This patch looks good to me.\n\nThanks. This patch needs a sign-off, by the way.\n\n> But that we should take it anyway regardless of that since it'll *also*\n> work on Linux with your patch, and this logic makes some sense whereas\n> the other one clearly didn't and just worked by pure accident of some\n> toolchain semantics that I haven't figured out yet.\n\nThat is curious and would be nice to know the answer to.\n\n"},{"id":"332397","messageId":"20171113065410.rb43utcbncy7ndrv@hashpling.org","threadId":"47182","inReplyTo":"xmqqmv3qal78.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2017-11-13T06:54:10Z","receivedAt":"2017-11-13T06:54:19Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Mon, Nov 13, 2017 at 12:53:15PM +0900, Junio C Hamano wrote:\n> \n> Thanks. This patch needs a sign-off, by the way.\n\nSigned-off-by: cbailey32@bloomberg.net\n\n(I can resend the full patch if required or if anyone requests futher\nchanges.\n\n> > But that we should take it anyway regardless of that since it'll *also*\n> > work on Linux with your patch, and this logic makes some sense whereas\n> > the other one clearly didn't and just worked by pure accident of some\n> > toolchain semantics that I haven't figured out yet.\n> \n> That is curious and would be nice to know the answer to.\n\nThe error that I was getting - if I remember the details of the very\nbrief debugging session that I performed - was an unaligned memory\naccess causing a SIGBUS in PCRE code whose function name contained 'jit'\nand which was being called indirectly from pcre_study.\n\nMy guess is that we are just exposing a pre-existing bug in our Solaris\nbuild of libpcre. Unaligned memory accesses on x86 / x86_64 \"only\" cause\nperformance issues rather than fatal signals so even if the same bug\nexists on Linux it probably has no noticeable effect (or at least no\nnoticed effect).\n\nCharles.\n"},{"id":"332493","messageId":"xmqqefp17gng.fsf@gitster.mtv.corp.google.com","threadId":"47182","inReplyTo":"20171113065410.rb43utcbncy7ndrv@hashpling.org","subject":"Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-14T02:12:03Z","receivedAt":"2017-11-14T02:12:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey writes:\n\n>> > But that we should take it anyway regardless of that since it'll *also*\n>> > work on Linux with your patch, and this logic makes some sense whereas\n>> > the other one clearly didn't and just worked by pure accident of some\n>> > toolchain semantics that I haven't figured out yet.\n>> \n>> That is curious and would be nice to know the answer to.\n>\n> The error that I was getting ...\n> My guess is that we are just exposing a pre-existing bug in our Solaris\n> build of libpcre.\n\nSorry, my question was not clear. I think you already mentioned the\nabove in the thread. What I was curious about was why Ævar was\nseeing that JIT disabled with NO_LIBPCRE1_JIT alone on his Linux\nsetup, i.e. namely this part from his message:\n\n *But* for some reason you still get away with that on Linux. I\n don't know why, but I assume the compiler toolchain is more lax\n for some reason than on Solaris.n\n\nIn any case, thanks for a fix; queued.\n"}]}