Page MenuHomeFreeBSD
Differential D10508 Diff 28009 lang/pocl/files/patch-lib_CL_devices_cpuinfo.c

Changeset View

Standalone View

View Options

lang/pocl/files/patch-lib_CL_devices_cpuinfo.c

--- lib/CL/devices/cpuinfo.c.orig 2016-11-20 11:31:19.521203000 +0100 --- lib/CL/devices/cpuinfo.c.orig 2017-04-05 14:15:40 UTC
+++ lib/CL/devices/cpuinfo.c 2016-11-20 11:29:24.502817000 +0100 +++ lib/CL/devices/cpuinfo.c
@@ -31,9 +31,13 @@ @@ -31,9 +31,13 @@
# include "vccompat.hpp" # include "vccompat.hpp"
#endif #endif
+#include <sys/types.h> +#include <sys/types.h>
+#include <sys/sysctl.h> +#include <sys/sysctl.h>
jbeichUnsubmitted
Done
Inline Actions

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

# CMakeLists.txt
include(CheckIncludeFiles)
check_include_files("sys/types.h;sys/sysctl.h" HAVE_SYS_SYSCTL_H)
/* config.h.in.cmake */
#cmakedefine HAVE_SYS_SYSCTL_H
/* lib/CL/devices/cpuinfo.c */
#ifdef HAVE_SYS_SYSCTL_H
#include <sys/types.h>
#include <sys/sysctl.h>
#endif
jbeich: Windows and Solaris lack `<sys/sysctl.h>`, so when upstreaming you may need include guards.
jbeichUnsubmitted
Done
Inline Actions

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

jbeich: If you do this move `#ifdef HAVE_SYS_SYSCTL_H` block **after** `#include "config.h"`. ;)
reznyAuthorUnsubmitted
Done
Inline Actions

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.

rezny: I noticed the MSVC check but this file looks to be rather Linux specific so I had not thought…
+ +
#include "config.h" #include "config.h"
#include "cpuinfo.h" #include "cpuinfo.h"
+#if 0 +#ifdef __linux__
const char* cpuinfo = "/proc/cpuinfo"; static const char* cpuinfo = "/proc/cpuinfo";
#define MAX_CPUINFO_SIZE 64*1024 #define MAX_CPUINFO_SIZE 64*1024
//#define DEBUG_POCL_CPUINFO //#define DEBUG_POCL_CPUINFO
@@ -152,8 +156,29 @@ pocl_cpuinfo_detect_max_clock_frequency( @@ -153,8 +157,29 @@ pocl_cpuinfo_detect_max_clock_frequency(
} }
return -1; return -1;
} }
+#endif +#elif defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || defined(__DragonFly__)
jbeichUnsubmitted
Done
Inline Actions

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:
https://github.com/DragonFlyBSD/DeltaPorts/tree/master/ports/lang/pocl
http://muscles.dragonflybsd.org/synth/logs/lang___pocl.log

jbeich: Prepend `defined(__DragonFly__) ||` as `hw.clockrate` is also supported there. OpenBSD can use…
reznyAuthorUnsubmitted
Done
Inline Actions

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.

rezny: I had just quickly changed preprocessor directives initially. Going back and looking at the…
+
+/** +/**
+ * Detects the number of parallel hardware threads supported by + * Detects the maximum clock frequency of the CPU.
+ * the CPU.
+ * + *
+ * @return The number of hardware threads. + * Assumes all cores have the same max clock freq.
+ *
+ * @return The clock frequency in MHz.
+ */ + */
+ int +int
+pocl_cpuinfo_detect_compute_unit_count() +pocl_cpuinfo_detect_max_clock_frequency()
jbeichUnsubmitted
Done
Inline Actions

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
==17273==ABORTING
jbeich: `value` string seems to lack terminating `NUL` character. ``` $ cc -g -fsanitize=address test.c…
reznyAuthorUnsubmitted
Done
Inline Actions

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: Well, that answers that question... sysctl(3) doesn't specifically address the NUL terminator…
+{ +{
+ int mib[2], nocpus; + //XXX PLEASE NOTE, THIS IS NOT TOO PORTABLE (AND/OR ACCURATE)!
+ size_t len; + const char mib[] = "hw.clockrate";
+ int clockrate;
+ size_t size = sizeof(clockrate);
jbeichUnsubmitted
Done
Inline Actions

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.

https://github.com/DragonFlyBSD/DragonFlyBSD/blob/6f1d2f41495d/lib/libc/gen/sysconf.c#L629
https://github.com/jsonn/src/blob/105a2837c72d/lib/libc/gen/sysconf.c#L370

jbeich: Replace with `#else` that calls `sysconf(_SC_NPROCESSORS_ONLN)` instead. All BSDs (and OS X)…
+
+ sysctlbyname(mib, (void *)&clockrate, &size, NULL, 0);
+ mib[0] = CTL_HW; + return clockrate;
+ mib[1] = HW_NCPU;
+ len = sizeof(nocpus);
+ sysctl(mib, 2, &nocpus, &len, NULL, 0);
+ return nocpus;
+} + }
+ +#endif
+#if 0
+#ifdef __linux__
/** /**
* Detects the number of parallel hardware threads supported by * Detects the number of parallel hardware threads supported by
* the CPU by parsing the cpuinfo. * the CPU by parsing the cpuinfo.
@@ -231,6 +256,27 @@ pocl_cpuinfo_detect_compute_unit_count() @@ -232,6 +257,19 @@ pocl_cpuinfo_detect_compute_unit_count()
} }
return -1; return -1;
} }
+#endif +#else
+
+/** +/**
+ * Detects the maximum clock frequency of the CPU. + * Detects the number of parallel hardware threads supported by
+ * the CPU.
+ * + *
+ * Assumes all cores have the same max clock freq. + * @return The number of hardware threads.
+ *
+ * @return The clock frequency in MHz.
+ */ + */
+int + int
+pocl_cpuinfo_detect_max_clock_frequency() +pocl_cpuinfo_detect_compute_unit_count()
+{ +{
+ //XXX PLEASE NOTE, THIS IS NOT TOO PORTABLE (AND/OR ACCURATE)! + return sysconf(_SC_NPROCESSORS_ONLN);
+ const char mib[] = "hw.clockrate";
+ size_t size = sizeof(int);
+ int clockrate;
+
+ sysctlbyname(mib, (void *)&clockrate, &size, NULL, 0);
+
+ return clockrate;
+ } +}
+#endif
#ifdef POCL_ANDROID #ifdef POCL_ANDROID
@@ -269,6 +315,7 @@ pocl_cpuinfo_get_cpu_name_and_vendor(cl_ @@ -270,6 +308,7 @@ pocl_cpuinfo_get_cpu_name_and_vendor(cl_
* short_name is in the .data anyways.*/ * short_name is in the .data anyways.*/
device->long_name = device->short_name; device->long_name = device->short_name;
+#if 0 +#ifdef __linux__
/* default vendor and vendor_id, in case it cannot be found by other means */ /* default vendor and vendor_id, in case it cannot be found by other means */
jbeichUnsubmitted
Done
Inline Actions

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

jbeich: Maybe fill `device->long_name` via `HW_MODEL` sysctl on all BSDs.
device->vendor = cpuvendor_default; device->vendor = cpuvendor_default;
if (device->vendor_id == 0) if (device->vendor_id == 0)
@@ -317,6 +364,7 @@ pocl_cpuinfo_get_cpu_name_and_vendor(cl_ @@ -318,6 +357,7 @@ pocl_cpuinfo_get_cpu_name_and_vendor(cl_
char *new_name = (char*)malloc (len); char *new_name = (char*)malloc (len);
snprintf (new_name, len, "%s-%s", device->short_name, start); snprintf (new_name, len, "%s-%s", device->short_name, start);
device->long_name = new_name; device->long_name = new_name;
+#endif +#endif
jbeichUnsubmitted
Done
Inline Actions

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

jbeich: `#elif defined(HW_MODEL)` and/or `#elif defined(HAVE_SYS_SYSCTL_H)` (see another comment) maybe…
} }