Bug 227723 - clang 6.0 breaks php56/opcache: Broken after clang 6.0 import (on CURRENT)
Summary: clang 6.0 breaks php56/opcache: Broken after clang 6.0 import (on CURRENT)
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: amd64 Any
: --- Affects Only Me
Assignee: Florian Smeets
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2018-04-23 16:30 UTC by lampa
Modified: 2018-05-25 04:03 UTC (History)
6 users (show)

See Also:
koobs: merge-quarterly+


Attachments
Patch created by dim@ (2.63 KB, patch)
2018-04-24 07:13 UTC, Florian Smeets
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lampa 2018-04-23 16:30:57 UTC
cc -v
FreeBSD clang version 6.0.0 (tags/RELEASE_600/final 326565) (based on LLVM 6.0.0)

uname -a
FreeBSD xxx 11.2-PRERELEASE FreeBSD 11.2-PRERELEASE #6 r332885: Mon Apr 23 13:32:20 CEST 2018 amd64

apache24 + php56-opcache-5.6.35 fails with SIGSEGV on simple <? phpinfo(); ?>

If opcache is recompiled/reinstalled using clang-5.0, it works.
Comment 1 Florian Smeets freebsd_committer 2018-04-24 07:13:22 UTC
Created attachment 192772 [details]
Patch created by dim@
Comment 2 Florian Smeets freebsd_committer 2018-04-24 07:15:28 UTC
I reported this to dim@ when clang 6 was merged to head. He created the patch I attached. It didn't work in my case, I'm still using USE_GCC in my lang/php56 port. I haven't had the time to dig further. Maybe you can try the patch.
Comment 3 lampa 2018-04-24 08:09:59 UTC
Patch seems to be ok. I've only changed installed header files in /usr/local/include/php/Zend, recompiled/reinstalled opcache, restarted apache and it runs.
I've reduced problem to:

#include <stdio.h>
#include <stdlib.h>

#define EX_TMP_VAR(ex, n)         ((temp_variable*)(((char*)(ex)) + ((int)(n))))
#define EX_TMP_VAR_NUM(ex, n)  (EX_TMP_VAR(ex, 0) - (1 + (n)))
#define VAR_NUM(v) ((unsigned int)(EX_TMP_VAR_NUM(0, 0) - EX_TMP_VAR(0, v)))

typedef struct TV {
        long tmp;
        char tmp3[30];
} temp_variable;

int num;

int main()
{
        printf("%x\n", VAR_NUM(-100));
        return 0;
}

cc -O2 t.c
f999999c
clang50 -O2 t.c
1
gcc -O2 t.c
1
Comment 4 lampa 2018-04-24 08:45:19 UTC
test case was for constant, which was optimized to immediate, here is another with variable:

#include <stdio.h>
#include <stdlib.h>

#define EX_TMP_VAR(ex, n)         ((temp_variable*)(((char*)(ex)) + ((int)(n))))
#define EX_TMP_VAR_NUM(ex, n)  (EX_TMP_VAR(ex, 0) - (1 + (n)))
#define VAR_NUM(v) ((unsigned int)(EX_TMP_VAR_NUM(0, 0) - EX_TMP_VAR(0, v)))

typedef struct TV {
        long tmp;
        char tmp3[30];
} temp_variable;

int num = -1000;

int main()
{
        printf("%x\n", VAR_NUM(num));
        return 0;
}

clang50 result is 18
clang60 result is 60000018

The only difference in generated code (clang50 first, clang60 second):

<       movslq  num(%rip), %rax
---
>       movl    num(%rip), %eax
        movq    $-40, %rcx
        subq    %rax, %rcx
        shrq    $3, %rcx


It looks like num is not sign extended before pointer arithmetic.
Comment 5 Florian Smeets freebsd_committer 2018-04-24 16:45:11 UTC
Over to maintainer. The patch works for the OP, I think in that case it should be committed to the port. I'll have to figure our why it didn't work for me, I probably applied it to the wrong poudriere ports tree.
Comment 6 Dimitry Andric freebsd_committer 2018-04-24 20:24:01 UTC
After some bisecting, it turns out this behavior changed due to the following upstream commit: https://reviews.llvm.org/rL313784 ("Remove offset size check in nullptr arithmetic handling") and https://reviews.llvm.org/rL313666 ("Teach clang to tolerate the 'p = nullptr + n' idiom used by glibc").

See also https://reviews.llvm.org/D37042.

What PHP5 is doing is, strictly speaking, undefined behavior, as adding numbers to null pointers is normally not allowed.  In the upstream commit, Andrew Kaylor has apparently tried to make this work, at least partially, for a few specific cases used in glibc.

PHP7 and later use a completely different way of storing these variable/number hybrids, which is not affected by this, as it apparently avoids any null pointer arithmetic.

I'll see if I can get some feedback from Andrew on this.
Comment 7 lampa 2018-04-25 12:19:04 UTC
That's true, if base nullptr is changed to any temp_variable* in VAR_NUM(), then generated code is identical and index is correctly sign extended. So this probably will be not fixed in clang and patch for php56 is more appropriate.
Comment 8 iron.udjin 2018-05-14 13:57:02 UTC
After I manually applied this patch to php56-5.6.36 on 11.2-PRERELEASE r333394 it fixed the problem with opcache. Please commit this patch.
Comment 9 commit-hook freebsd_committer 2018-05-14 14:25:09 UTC
A commit references this bug:

Author: flo
Date: Mon May 14 14:24:14 UTC 2018
New revision: 469895
URL: https://svnweb.freebsd.org/changeset/ports/469895

Log:
  Prevent php 5.6 (opcache) from segfaulting when compiled with clang 6.0

  PR:             227723
  Submitted by:   dim
  Reported by:    flo, lampa@fit.vutbr.cz
  Approved by:    maintainer timeout
  MFH:            2018Q2

Changes:
  head/lang/php56/Makefile
  head/lang/php56/files/patch-Zend_zend__compile.h
  head/lang/php56/files/patch-Zend_zend__execute.h
Comment 10 Florian Smeets freebsd_committer 2018-05-14 18:15:10 UTC
Patch was committed to the tree.
Comment 11 commit-hook freebsd_committer 2018-05-24 19:54:15 UTC
A commit references this bug:

Author: flo
Date: Thu May 24 19:53:43 UTC 2018
New revision: 470806
URL: https://svnweb.freebsd.org/changeset/ports/470806

Log:
  MFH: r469895

  Prevent php 5.6 (opcache) from segfaulting when compiled with clang 6.0

  PR:             227723
  Submitted by:   dim
  Reported by:    flo, lampa@fit.vutbr.cz
  Approved by:    maintainer timeout

  Approved by:	ports-secteam (riggs)

Changes:
_U  branches/2018Q2/
  branches/2018Q2/lang/php56/Makefile
  branches/2018Q2/lang/php56/files/patch-Zend_zend__compile.h
  branches/2018Q2/lang/php56/files/patch-Zend_zend__execute.h
Comment 12 Kubilay Kocak freebsd_committer freebsd_triage 2018-05-25 04:03:28 UTC
Assign to committer that resolved. Track merge to quarterly.