Page MenuHomePhabricator

[Driver] Prioritize SYSROOT/usr/include over RESOURCE_DIR/include on linux-musl
ClosedPublic

Authored by MaskRay on Aug 3 2019, 9:42 AM.

Details

Summary

On a musl-based Linux distribution, stdalign.h stdarg.h stdbool.h stddef.h stdint.h stdnoreturn.h are expected to be provided by musl (/usr/include), instead of RESOURCE_DIR/include.
Reorder RESOURCE_DIR/include to fix the search order problem.
(Currently musl doesn't provide stdatomic.h. stdatomic.h is still found in RESOURCE_DIR/include.)

gcc on musl has a similar search order:

/usr/lib/gcc/x86_64-alpine-linux-musl/8.3.0/../../../../include/c++/8.3.0
/usr/lib/gcc/x86_64-alpine-linux-musl/8.3.0/../../../../include/c++/8.3.0/x86_64-alpine-linux-musl
/usr/lib/gcc/x86_64-alpine-linux-musl/8.3.0/../../../../include/c++/8.3.0/backward
/usr/local/include
/usr/include/fortify
/usr/include
/usr/lib/gcc/x86_64-alpine-linux-musl/8.3.0/include

This is different from a glibc-based distribution where RESOURCE_DIR/include is placed before SYSROOT/usr/include.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Aug 3 2019, 9:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2019, 9:42 AM
dalias added a comment.Aug 4 2019, 7:13 PM

Just chiming in to confirm that the requested change is correct for musl. musl does not support use/mixing of compiler-provided std headers with its headers, and intentionally has no mechanism for communicating with such headers as to which types have already been defined or still need to be defined. If the current include order, with clang's headers before the libc ones, works in some situations, it's only by accident.

My understanding is that most if not all of the BSDs also work this way; GCC has their libc header dirs ordered before the GCC include dir. So it probably makes sense to do the same change for them.

phosek added a comment.Aug 5 2019, 2:11 PM

This looks reasonable to me; I'd prefer to clean up Clang's internal headers to be compatible with both FreeBSD's and musl's standard headers as suggested in http://lists.llvm.org/pipermail/llvm-dev/2019-August/134353.html, but I'm not sure how feasible or doable that is.

dalias added a comment.Aug 5 2019, 2:57 PM

It's not just infeasible; it's impossible by design. musl's headers do not support any contract with compiler-provided versions of the std headers, and won't. Attempting to use them in place of the libc ones is unsupported usage and will periodically break.

MaskRay added a reviewer: rnk.Aug 5 2019, 8:12 PM
phosek accepted this revision.Aug 5 2019, 10:24 PM

LGTM

This revision is now accepted and ready to land.Aug 5 2019, 10:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 11:24 PM

Can we bring this to 9.x release branch as well please ?

one issue I now see is that when we have some thing like

static const struct {

        const wchar_t *name;
        int (*func)(EditLine *, int, const wchar_t **);
} cmds[] = {
        { L"bind",              map_bind        },
        { L"echotc",            terminal_echotc },
        { L"edit",              el_editmode     },
        { L"history",           hist_command    },
        { L"telltc",            terminal_telltc },
        { L"settc",             terminal_settc  },
        { L"setty",             tty_stty        },
        { NULL,                 NULL            }
};

clang complains

warning: incompatible pointer types initializing 'const wchar_t *' (aka 'const long *') with an expression of type 'int [5]' [-Wincompatible-pointer-types]

{ L"bind",              map_bind        },
  ^~~~~~~

it seems L"..." prefix is treated as int where as in musl wchar_t is of long int type

dalias added a comment.Sep 9 2019, 4:44 AM

That's a separate issue of clang having a slight types-ABI mismatch with some 32-bit archs whose original ABIs used long instead of int for wchar_t. The wrong header order makes it quickly apparent, but these are independent; wrong header order is still wrong and will break (other) things even without this type mismatch. Backport of the fix would be much appreciated.

MaskRay added a subscriber: hans.Sep 9 2019, 4:51 AM

@hans Can this be put on your list of things for 9.0.1? I searched for [meta] 9.0.1 on bugs.llvm.org but can't find a release blocker bug so I have to bother you directly...

hans added a comment.Sep 9 2019, 5:07 AM

@hans Can this be put on your list of things for 9.0.1? I searched for [meta] 9.0.1 on bugs.llvm.org but can't find a release blocker bug so I have to bother you directly...

Right, the bug hasn't been filed yet. I've put it on my local list. Thanks!

That's a separate issue of clang having a slight types-ABI mismatch with some 32-bit archs whose original ABIs used long instead of int for wchar_t. The wrong header order makes it quickly apparent, but these are independent; wrong header order is still wrong and will break (other) things even without this type mismatch. Backport of the fix would be much appreciated.

@dalias I agree its an issue that comes to fore due to this change and was latent, this however now fails to build libedit, which is a prerequisite for building clang itself, so its kind of pesky problem now.