On Wed, Jul 10, 2019 at 6:25 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > > > Am 09.07.2019 um 20:14 schrieb Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>: > > > > On Mon, Jul 8, 2019 at 8:01 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote: > >> > >>> Am 05.07.2019 um 16:22 schrieb Daniel Borkmann <daniel@xxxxxxxxxxxxx>: > >>> > >>> On 07/01/2019 08:40 PM, Ilya Leoshkevich wrote: > >>>> Am 01.07.2019 um 17:31 schrieb Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>: > >>>>> Do we still need clang | llc pipeline with new clang? Could the same > >>>>> be achieved with single clang invocation? That would solve the problem > >>>>> of not detecting pipeline failures. > >>>> > >>>> I’ve experimented with this a little, and found that new clang: > >>>> > >>>> - Does not understand -march, but -target is sufficient. > >>>> - Understands -mcpu. > >>>> - Understands -Xclang -target-feature -Xclang +foo as a replacement for > >>>> -mattr=foo. > >>>> > >>>> However, there are two issues with that: > >>>> > >>>> - Don’t older clangs need to be supported? For example, right now alu32 > >>>> progs are built conditionally. > >>> > >>> We usually require latest clang to be able to test most recent features like > >>> BTF such that it helps to catch potential bugs in either of the projects > >>> before release. > >>> > >>>> - It does not seem to be possible to build test_xdp.o without -target > >>>> bpf. > >>> > >>> For everything non-tracing, it does not make sense to invoke clang w/o > >>> the -target bpf flag, see also Documentation/bpf/bpf_devel_QA.rst +573 > >>> for more explanation, so this needs to be present for building test_xdp.o. > >> > >> I'm referring to the test introduced in [1]. test_xdp.o might not be an > >> ideal target, but even if it's replaced with a more suitable one, the > >> llc invocation would still be required. So I could redo the patch as > >> follows: > >> > >> - Replace test_xdp.o with get_cgroup_id_kern.o, use an intermediate .bc > >> file. > >> - Use clang without llc for all other eBPF programs. > >> - Split out Kbuild include and order-only prerequisites. > >> > >> What do you think? > > > > How about just forcing llc to fail as well like this: > > > > (clang <whatever> || echo "clain failed") | llc <whatever> > > > > While not pretty, it will get us what we need with very clear > > messaging as well. E.g.: > > > > progs/test_btf_newkv.c:21:37: error: expected identifier > > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > > ^ > > progs/test_btf_newkv.c:21:1: warning: type specifier missing, defaults > > to 'int' [-Wimplicit-int] > > PF_ANNOTATE_KV_PAIR(btf_map_legacy, int, struct ipv_counts); > > ^ > > 1 warning and 1 error generated. > > llc: error: llc: <stdin>:1:1: error: expected top-level entity > > clang failed > > ^ > > While this would definitely work at least in my scenario, what about the > following hypothetical cases? > > - clang manages to output something before exiting with nonzero rc > - future llc version exits with zero rc when given "clang failed" or any > other arbitrary string as an input (perhaps, with just a warning?) This seems very far-fetched. While I can imagine partial output from clang, I can't imagine accepting some garbage as an input for llc and yet returning zero exit code. That would be major regression, so I wouldn't worry about it. > > Come to think of it, what are the downsides of having intermediate > bitcode files? While I did not run into this yet, I could imagine it > might be even useful from time to time to inspect them. Main downside is complication of Makefile, which is not the simplest to follow already. So if we can solve problem in simpler way without adding more complexity, that would be my preferred way. If someone wants bitcode, when you do `make`, you see all the commands that are being run, so just copy/paste parts you care about (i.e., clang invocation). That's what I do when I need to repro something for single .o file, for instance. >
