In D36388#836337, @RKSimon wrote:

@dim What effect does this patch have on PR34080? The changes in pr34080.ll aren't looking hopeful

In D33900#824172, @thakis wrote:

dim: Does putting the target listing behind a different flag work for you? Which problem are you trying to solve here?

Also note that it is only added to the --version output, not the -v output (the former is really a "verbose" version of the latter):

$ clang -v
clang version 5.0.0 (trunk 308421)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /share/dim/llvm/308421-trunk-freebsd12-amd64-ninja-rel-1/bin

In D33900#818957, @thakis wrote:

Sorry, I just noticed this weeks later. Why are we adding this to --version instead of adding some new flag for printing this? When I pass --version, I'm usually interested in clang's version and don't need a screenful of other information below it (which makes the output I do care about scroll off the screen).

Yep, LGTM. That would hopefully also allow this to build with recent versions of gcc without warnings.

I went for using GetSystemInfo() on Windows, pagesize() everywhere else, in the end. I think that is the most complete version. I'm keeping an eye on the build bots, if anything strange happens, I will revert it ASAP.

Submitted https://bugs.llvm.org/show_bug.cgi?id=33820

This commit causes link failures on FreeBSD/AArch64, where it apparently messes something up in libc.so.7's symbols, and executables can't link against it anymore, because of 'undefined' symbols. (I haven't seen problems yet on X86.)

In rL307100#105390, @dim wrote:

So for some reason, the OutputSectionCommand *Cmd = findSectionCommand(".ARM.exidx"); line earlier in that function results in a valid OutputSectionCommand object, but with a null Sec member.

This commit causes segfaults when linking the FreeBSD GENERIC kernel for aarch64:

(gdb) run
Starting program: /home/dim/obj/llvm-trunk-r307100/bin/ld.lld -m aarch64elf -Bdynamic -T /home/dim/src/clang500-import/sys/conf/ldscript.arm64 --build-id=sha1 --no-warn-mismatch --warn-common --export-dynamic --dynamic-linker /red/herring -o kernel.full -X locore.o cam.o cam_compat.o cam_iosched.o cam_periph.o cam_queue.o cam_sim.o c [... lots of objects ...] config.o env.o hints.o vnode_if.o hack.pico vers.o
[New LWP 100238 of process 41586]
[New LWP 100249 of process 41586]
[New LWP 100257 of process 41586]
[New LWP 100264 of process 41586]
[New LWP 100269 of process 41586]
[New LWP 100288 of process 41586]
[New LWP 100296 of process 41586]
[New LWP 100297 of process 41586]
[New LWP 100301 of process 41586]
[New LWP 100302 of process 41586]
[New LWP 100309 of process 41586]
[New LWP 100323 of process 41586]
[New LWP 100333 of process 41586]
[New LWP 100359 of process 41586]
[New LWP 100363 of process 41586]
[New LWP 100375 of process 41586]
[New LWP 100393 of process 41586]
[New LWP 100413 of process 41586]
[New LWP 100424 of process 41586]
[New LWP 100434 of process 41586]
[New LWP 100464 of process 41586]
[New LWP 100474 of process 41586]
[New LWP 100842 of process 41586]
[New LWP 101856 of process 41586]
[New LWP 101859 of process 41586]
[New LWP 101861 of process 41586]
[New LWP 101865 of process 41586]
[New LWP 101867 of process 41586]
[New LWP 101871 of process 41586]
[New LWP 101875 of process 41586]
[New LWP 101884 of process 41586]
[New LWP 101887 of process 41586]

Pinging this again. Should we go for this minimal change, or do we want the GetSystemInfo call? :)

Minimized changes:

• Rename PAGE_SIZE macro to KMP_PAGE_SIZE
• Remove untested Windows code and revert to just returning KMP_PAGE_SIZE
• For all other OSes, return getpagesize()

In D35072#801197, @AndreyChurbanov wrote:

OK, thanks, I agree system macros should not be re-defined.

Then, maybe rename it into non-system one, like KMP_PAGE_SIZE? I am just scared that a lot of platforms need testing because of so little change, while there should be a way to avoid the testing, I think.

In D35072#801129, @AndreyChurbanov wrote:

Isn't it safer to just put the definition of PAGE_SIZE under "#ifndef PAGE_SIZE"?

This would not require to test all the platforms affected except Linux.

Move inclusion of windows.h a bit higher, and change SYSTEMINFO to SYSTEM_INFO.

Use getpagesize() on all platforms, and add a Windows implementation of it.

Apparently KMP_OS_CNK is for Blue Gene/Q, which seems to be a Linux-like, maybe @hfinkel might know whether this OS supports getpagesize()? If so, the case can be simplified to providing a getpagesize() only for Windows, then defining KMP_GET_PAGE_SIZE() to getpagesize() unconditionally.

Note the TODO case also covers KMP_OS_CNK, of which I have no idea what it is. So the 0x4000 case must stay in there anyway.

In D35072#800989, @zturner wrote:

There's a comment indicating that we should find the correct function on Windows and use it, but in the description of this review you mention that you found it in the SYSTEM_INFO structure. So why not use it?

LGTM

This causes a regression, which I have reported in https://bugs.llvm.org/show_bug.cgi?id=33653.

Use CMAKE_DL_LIBS instead. This is in CMake since at least 2012, if not before.

In D34632#790913, @jlpeyton wrote:

CMake has a variable CMAKE_DL_LIBS which I believe contains the information you want. It essentially gets defined to "-ldl" on unices that need it and "" on platforms which don't need it.

Use a std::function object instead of a plain function pointer.

Alternatively, as a lower impact change, I could just add the code from printRegisteredTargetsForVersion to tools/clang/lib/Driver/Driver.cpp, but it's a little ugly...

In D33600#772084, @krytarowski wrote:

Why does this code live in a Linux file? Are there plans to split it out, rename?

Ping.

Note that this commit can cause the following failure, as @joerg also found out:

Unhandled cast
UNREACHABLE executed at include/llvm/CodeGen/BasicTTIImpl.h:590!

Minimized test case:

/* clang -cc1 -triple x86_64 -S -O2 -vectorize-loops -vectorize-slp testcase-r303763.c */
char a[1];
long b;
unsigned c;
void d(void);
void d() {
  for (; c < b; c++)
    if (a[c])
      a[c] = ',';
}

With that last patch, I get the following test results, which are not greatly different from those on x86:

Actually the <sys/exec.h> header is still needed for the ps_string usage later in the file, so the diff becomes:

--- lib/sanitizer_common/sanitizer_linux.cc     (revision 303869)
+++ lib/sanitizer_common/sanitizer_linux.cc     (working copy)
@@ -62,8 +62,6 @@
 #if SANITIZER_FREEBSD
 #include <sys/exec.h>
 #include <sys/sysctl.h>
-#include <vm/vm_param.h>
-#include <vm/pmap.h>
 #include <machine/atomic.h>
 extern "C" {
 // <sys/umtx.h> must be included after <errno.h> and <sys/types.h> on

It turns out I added the <vm/pmap.h> inclusion in rL263157, but it and the two other headers are not needed anymore, as the following diff at least makes it compile:

--- lib/sanitizer_common/sanitizer_linux.cc     (revision 303869)
+++ lib/sanitizer_common/sanitizer_linux.cc     (working copy)
@@ -60,10 +60,7 @@
 #include <unistd.h>

Unfortunately it does not yet compile on FreeBSD 12/arm64, as there is a problem with the system header <machine/pmap.h>:

In file included from /home/dim/src/llvm-trunk/projects/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc:66:
In file included from /usr/include/vm/pmap.h:90:
/usr/include/machine/pmap.h:57:28: error: unknown type name 'vm_page_t'
void pmap_page_set_memattr(vm_page_t m, vm_memattr_t ma);
                           ^
/usr/include/machine/pmap.h:57:41: error: unknown type name 'vm_memattr_t'
void pmap_page_set_memattr(vm_page_t m, vm_memattr_t ma);
                                        ^
/usr/include/machine/pmap.h:66:2: error: unknown type name 'vm_memattr_t'
        vm_memattr_t            pv_memattr;
        ^
3 errors generated.

I've now tried commenting out the inclusion of <vm/pmap.h>, to side-step the issue. It's still building...

In D33600#766133, @vitalybuka wrote:

Do we have public build bot for FreeBSD?

I accidentally left a debugging aid in place. Remove it.

LGTM, though it would be nice if we implemented gcc's extension for this at some point... ;)

Hmm, it's a bit weird that lib/builtins doesn't have its own .clang-format file. Is this supposed to have LLVM or Google style? The style across various files seems to be wildly different.

As I found out in rL303015, the KERN_PROC_PATHNAME has one drawback: if an executable file has multiple hard links, you will get just one of its filenames as the result. Since that filename is more or less randomly chosen, it does *not* have to correspond to the actual argv[0] the executable was invoked with. If that does not matter, this approach is fine, though.

• Move sys/sysctl.h include to a separate block, specifically for (Free|Net)BSD
• Remove NetBSD from several #if statements
• Use correct mib for NetBSD to retrieve process pathname

Ping :)

Simplify test case a bit.

Note: I copied some of the test case from rL227097, which had a similar case of a too short buffer.

In D32120#734641, @joerg wrote:

I think a better idea would be to just allow this tests to fail with EPERM.

Also add NetBSD. At some point, we might want to have a global
LLVM_ON_BSD define to make these easier. :)