In D37415#880595, @MatzeB wrote:

In D37415#880589, @ppenzin wrote:

It is a FreeBSD-ism: non-system libraries and their headers go into /usr/local, which is not on default search paths. But there might be a more elegant way to do it, I will take a look.

That I understand. I'm just surprised the libxml stuff doesn't end up in /usr/local/include/libxml2/libxml/*.h and people need to explicitely state -I /usr/local/include/libxml2 (or rather pkg-config --cflags libxml2). Or is the comment not up-to-date and this the /usr/local/include is needed for something else than libxml2?

Actually I see that we also get this warning on our .section ".note.GNU-stack","",@progbits directives, which are in most of our hand-written .S files. Since the type of that section is progbits, this fix won't work for it.

In D6379#95991, @dblaikie wrote:

Should we add that filter elsewhere rather than here? (eg: we shouldn't be adding such non-text sections into the Sections map if they're not part of teh DWARF anyway, right?)

Pinging, since it has come up again on the FreeBSD mailing lists, now this warning is emitted thousands of times. Adding Ed as a reviewer.

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.