On Tue, Dec 11 2018, Carlo Arenas wrote:
> On Mon, Dec 10, 2018 at 12:24 AM Ævar Arnfjörð Bjarmason > <ava...@gmail.com> wrote: >> On Mon, Dec 10 2018, brian m. carlson wrote: >> > Considering that some Linux users use PaX kernels with standard >> > distributions and that most BSD kernels can be custom-compiled with a >> > variety of options enabled or disabled, I think this is something we >> > should detect dynamically. >> >> Right. I'm asking whether we're mixing up cases where it can always be >> detected at compile-time on some systems v.s. cases where it'll >> potentially change at runtime. > > the closer we come to a system specific issues is with macOS where the > compiler (in some newer versions) is allocating the memory using the > MAP_JIT flag, which seems was originally meant to be only used in iOS > and has the strange characteristic of failing the mmap for versions > older than 10.14 if it was called more than once. > > IMHO as brian pointed out, this is better done at runtime. Sure. Just something I was wondering since it wasn't clear from the patch. Makes sense, if it's always runtime (or not worth the effort to divide the two) let's do that. >> >> I'm inclined to suggest that we should have another ifdef here for "if >> >> JIT fails I'd like it to die", so that e.g. packages I build (for >> >> internal use) don't silently slow down in the future, only for me to >> >> find some months later that someone enabled an overzealous SELinux >> >> policy and we swept this under the rug. >> > >> > My view is that JIT is a nice performance optimization, but it's >> > optional. I honestly don't think it should even be exposed through the >> > API: if it works, then things are faster, and if it doesn't, then >> > they're not. I don't see the value in an option for causing things to be >> > broken if someone improves the security of the system. >> >> For many users that's definitely the case, but for others that's like >> saying a RDBMS is still going to be functional if the "ORDER BY" >> function degrades to bubblesort. The JIT improves performance my >> multi-hundred percents sometimes, so some users (e.g. me) rely on that >> not being silently degraded. > > the opposite is also true, specially considering that some old > versions of pcre result in a segfault instead of an error message and > therefore since there is no way to disable JIT, the only option left > is not to use `git grep -P` (or the equivalent git log call) Right, of course it segfaulting is a bug... >> So I'm wondering if we can have something like: >> >> if (!jit) >> if (must_have_jit) >> BUG(...); // Like currently >> else >> fallback(); // new behavior > > I am wondering if something like a `git doctor` command might be an > interesting alternative to this. > > This way we could (for ex: in NetBSD) give the user a hint of what to > do to make their git grep -P faster when we detect we are running the > fallback, and might be useful as well to provide hints for > optimizations that could be used in other cases (probably even > depending on the size of the git repository) Such a command has been discussed before on-list. I think it's a good idea for the more fuzzy things like optimization suggests, but for the case of expecting something at compile-time where not having that at runtime is a boolean state it's nicer to just die with a BUG(...). > For your use case, you just need to add a crontab that will trigger an > alarm if this command ever mentions PCRE ...The reason I'd like it to die is because it neatly and naturally integrates with all existing test infrastructure I have. I.e. build package, run stress tests on all sorts of machines, see that it passes, and SELinux isn't ruining it or whatever. I know this works now (I always get PCRE v2 JIT) because it doesn't die or segfault. I'd like not to have it regress to having worse performance. Having a cronjob to test for "does PCRE v2 JIT still work?" is not as easy & isn't a drop-in solution.
