Page MenuHomeFreeBSD

Update lang/pocl to 0.14 and build with llvm40 by default

Authored by rezny on Apr 26 2017, 12:37 PM.



With the release of pocl 0.14 it is now compatible with llvm39 and llvm40 so with this update the port now defaults to 40 and will follow MESA_LLVM_VER if set. The port switches to using CMake because that is what this release supports; there are no autotools files in 0.14. Also, i386 has been unblocked as it appears to work and upstream appears to support even non-x86. Maintainer timed out in PR 218332.

Test Plan

Poudriere 11.0 amd64/i386 OK

Diff Detail

rP FreeBSD ports repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Trim obsolete USES=libtool:keepla

Update based on feedback in the PR.

jbeich added inline comments.
37 ↗(On Diff #27968)

Did you forget to drop no longer used DEBUG option?

21 ↗(On Diff #27968)

Prepend defined(__DragonFly__) || as hw.clockrate is also supported there. OpenBSD can use HW_CPUSPEED instead but out of scope for now.

pocl-0.13 builds fine on DragonFly without patches. Let's keep pocl-0.14 the same way:

51 ↗(On Diff #27968)

Replace with #else that calls sysconf(_SC_NPROCESSORS_ONLN) instead. All BSDs (and OS X) support hw.ncpu. However, on NetBSD hw.ncpu is different from hw.ncpuonline.

80 ↗(On Diff #27968)

Maybe fill device->long_name via HW_MODEL sysctl on all BSDs.

rezny marked 2 inline comments as done.

Revise patch-lib_CL_devices_cpuinfo.c

37 ↗(On Diff #27968)

Cmake responds to the DEBUG option without any extra setting. Try the option and notice it builds Debug instead of Release configuration.

21 ↗(On Diff #27968)

I had just quickly changed preprocessor directives initially. Going back and looking at the code within, there's probably a more accurate answer under dev.cpu than what hw.clockrate offers but I do not know how portable that would be.

I'm a bit biased against DEBUG but the rest looks fine.

37 ↗(On Diff #27968)

Does it provide anything more than symbols (e.g. assertions, statistics, verbosity, slow path)? Otherwise, WITH_DEBUG=1 facilitiy is generic for every port that respects CFLAGS/STRIP. To get useful stacktraces one still needs to build dependencies with symbols. And default flags may not be enough e.g., crashes may disappear without -O level or be confusing without -fsanitize=address.

This revision is now accepted and ready to land.May 4 2017, 12:27 PM

I'm a bit biased against DEBUG but the rest looks fine.

I had left DEBUG in place because it was there and was doing something but if it shouldn't be there then DEBUG can easily go; WITH_DEBUG=1 does the same thing as setting the option.

rezny edited edge metadata.
rezny marked 5 inline comments as done.

Improve platform support as suggested and drop DEBUG.

This revision now requires review to proceed.May 4 2017, 4:15 PM
8 ↗(On Diff #28026)

Windows and Solaris lack <sys/sysctl.h>, so when upstreaming you may need include guards.

# CMakeLists.txt
check_include_files("sys/types.h;sys/sysctl.h" HAVE_SYS_SYSCTL_H)
/* */
#cmakedefine HAVE_SYS_SYSCTL_H
/* lib/CL/devices/cpuinfo.c */
#include <sys/types.h>
#include <sys/sysctl.h>
42 ↗(On Diff #28026)

value string seems to lack terminating NUL character.

$ cc -g -fsanitize=address test.c
$ ./a.out
==17273==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60f0000000ed at pc 0x00000042d769 bp 0x7fffffffde30 sp 0x7fffffffd5d8
READ of size 174 at 0x60f0000000ed thread T0
    #0 0x42d768 in __interceptor_strlen (/home/foo/a.out+0x42d768)
    #1 0x80125a314 in vsscanf_l /usr/src/lib/libc/stdio/vsscanf.c:69:23
    #2 0x80125a314 in vsscanf /usr/src/lib/libc/stdio/vsscanf.c:77
    #3 0x43d24d in sscanf (/home/foo/a.out+0x43d24d)
    #4 0x499bba in pocl_cpuinfo_detect_max_clock_frequency /home/foo/test.c:26:7
    #5 0x499cfa in main /home/foo/test.c:40:18
    #6 0x40d1af in _start /usr/src/lib/csu/amd64/crt1.c:72:7

0x60f0000000ed is located 0 bytes to the right of 173-byte region [0x60f000000040,0x60f0000000ed)
allocated by thread T0 here:
    #0 0x48555c in malloc (/home/foo/a.out+0x48555c)
    #1 0x499b5e in pocl_cpuinfo_detect_max_clock_frequency /home/foo/test.c:23:23
    #2 0x499cfa in main /home/foo/test.c:40:18
    #3 0x40d1af in _start /usr/src/lib/csu/amd64/crt1.c:72:7
    #4 0x8006ddfff  (<unknown module>)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/foo/a.out+0x42d768) in __interceptor_strlen
Shadow bytes around the buggy address:
  0x4c1dffffffc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c1dffffffd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c1dffffffe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c1dfffffff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x4c1e00000000: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x4c1e00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00[05]fa fa
  0x4c1e00000020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c1e00000030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c1e00000040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c1e00000050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x4c1e00000060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
91 ↗(On Diff #28026)

#elif defined(HW_MODEL) and/or #elif defined(HAVE_SYS_SYSCTL_H) (see another comment) maybe less assuming about portability.

8 ↗(On Diff #28026)

If you do this move #ifdef HAVE_SYS_SYSCTL_H block after #include "config.h". ;)

rezny added inline comments.
8 ↗(On Diff #28026)

I noticed the MSVC check but this file looks to be rather Linux specific so I had not thought about portability beyond there, not that I had put any thought to upstreaming, but easy enough to add those checks.

42 ↗(On Diff #28026)

Well, that answers that question... sysctl(3) doesn't specifically address the NUL terminator but implies by example that it would be provided and accounted for in the size. On the other hand, I have seen manual NUL termination of strings from sysctl() often enough to be suspicious of the simplistic example.

rezny marked 3 inline comments as done.

Terminate strings from sysctl and add header include guard for portability.

This revision is now accepted and ready to land.May 5 2017, 11:03 PM
This revision was automatically updated to reflect the committed changes.