From owner-freebsd-toolchain@freebsd.org Mon Feb 29 01:41:06 2016 Return-Path: Delivered-To: freebsd-toolchain@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DB0D1AAD5D0 for ; Mon, 29 Feb 2016 01:41:06 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: from asp.reflexion.net (outbound-mail-211-154.reflexion.net [208.70.211.154]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 9BEB1192 for ; Mon, 29 Feb 2016 01:41:05 +0000 (UTC) (envelope-from markmi@dsl-only.net) Received: (qmail 27301 invoked from network); 29 Feb 2016 01:40:58 -0000 Received: from unknown (HELO mail-cs-02.app.dca.reflexion.local) (10.81.19.2) by 0 (rfx-qmail) with SMTP; 29 Feb 2016 01:40:58 -0000 Received: by mail-cs-02.app.dca.reflexion.local (Reflexion email security v7.80.0) with SMTP; Sun, 28 Feb 2016 20:40:53 -0500 (EST) Received: (qmail 21805 invoked from network); 29 Feb 2016 01:40:52 -0000 Received: from unknown (HELO iron2.pdx.net) (69.64.224.71) by 0 (rfx-qmail) with SMTP; 29 Feb 2016 01:40:52 -0000 X-No-Relay: not in my network X-No-Relay: not in my network X-No-Relay: not in my network X-No-Relay: not in my network Received: from [192.168.1.8] (c-76-115-7-162.hsd1.or.comcast.net [76.115.7.162]) by iron2.pdx.net (Postfix) with ESMTPSA id 05472B1E001; Sun, 28 Feb 2016 17:40:54 -0800 (PST) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2104\)) Subject: Re: clang 3.8.0 can mess up __builtin_dwarf_cfa (), at least for TARGET_ARCH=armv6, powerpc, powerpc64: a bug 207325 update From: Mark Millard In-Reply-To: <462637FA-6FD2-4421-8F0C-0DF565E94FA6@dsl-only.net> Date: Sun, 28 Feb 2016 17:40:58 -0800 Cc: freebsd-arm , FreeBSD PowerPC ML , FreeBSD Toolchain Content-Transfer-Encoding: quoted-printable Message-Id: <8D92D76D-9FBC-45F6-A20D-0C2A8B38D291@dsl-only.net> References: <83B8741C-B4C9-4EFB-A3B4-473F8F165984@dsl-only.net> <80EA4460-E842-46F5-B006-2A83FBBEE845@dsl-only.net> <366B67F9-6A14-4906-8545-1B57A3FF53B8@dsl-only.net> <20160228083934.GA60222@vlakno.cz> <22AD8E4F-B3F2-455E-8EBE-2C70E428D44A@dsl-only.net> <462637FA-6FD2-4421-8F0C-0DF565E94FA6@dsl-only.net> To: Roman Divacky X-Mailer: Apple Mail (2.2104) X-BeenThere: freebsd-toolchain@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Maintenance of FreeBSD's integrated toolchain List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Feb 2016 01:41:07 -0000 Looking some at clang/llvm history shows releases/branches: V2.6 did not have "case Builtin::BI__builtin_dwarf_cfa". V2.7 did have "case Builtin::BI__builtin_dwarf_cfa" but = PPCTargetLowering::LowerFRAMEADDR ignored the argument. V2.8 had PPCTargetLowering::LowerFRAMEADDR using its argument (as a = frame depth, not a byte offset). The apparently incorrect (not matching g++ frame depth returned) = comments, naming, and value (when viewed as a frame depth) for "case = Builtin::BI__builtin_dwarf_cfa" started in V2.7 and continues to this = day. That is a lot of time for various dependencies on the clang = (mis)definition to accumulate across everything that uses clang. It may be that limiting any change to specific TARGET_ARCH's for FreeBSD = is appropriate. FreeBSD would likely need to list the appropriate = TARGET_ARCH's, likely including powerpc and powerpc64 since clang before = 3.8.0 was not in use for buildworld for powerpc/powerpc64. Still this may have consequences for ports that use clang and might = reference clang-compiled __builtin_dwarf_cfa() use, possibly from a = lang/clang* instead of the system clang. My guess is that the = interoperability with being able to use g++ vintages as well may lead to = (modern?) lang/clang*'s tracking the fix for FreeBSD TARGET_ARCH's that = are fixed. I can ignore all this and build a system based on using 1 as the frame = depth just to test, just as a matter of proof of concept for powerpc. = (Powerpc64 hits a system libgcc_s defect and so needs more before C++ = exceptions can be tested overall.) =3D=3D=3D Mark Millard markmi at dsl-only.net On 2016-Feb-28, at 2:20 PM, Mark Millard wrote: In /usr/src/contrib/llvm/tools/clang/lib/CodeGen/CGBuiltin.cpp there is: > case Builtin::BI__builtin_dwarf_cfa: { > // The offset in bytes from the first argument to the CFA. > // > // Why on earth is this in the frontend? Is there any reason at > // all that the backend can't reasonably determine this while > // lowering llvm.eh.dwarf.cfa()? > // > // TODO: If there's a satisfactory reason, add a target hook for > // this instead of hard-coding 0, which is correct for most = targets. > int32_t Offset =3D 0; >=20 > Value *F =3D CGM.getIntrinsic(Intrinsic::eh_dwarf_cfa); > return RValue::get(Builder.CreateCall(F, > llvm::ConstantInt::get(Int32Ty, = Offset))); > } I would have guessed that the internal argument was how many frames away = on the stack to go from what 0 produces (high er address direction). = g++'s __builtin_dwarf_cfa() returns the address for the next frame = compared to clang 3.8.0 (higher address direction). I'd call that more of a frame depth than an offset. .eh_frame and its = cfa material use offset terminology as byte offsets. And the comments = above talk of an offset in bytes --but "next frame" distances in bytes = would not be constant. Looking at a use of LowerFRAMEADDR in a LowerRETURNADDR, for example, > SDValue ARMTargetLowering::LowerRETURNADDR(SDValue Op, SelectionDAG = &DAG) const{ > . . . > EVT VT =3D Op.getValueType(); > SDLoc dl(Op); > unsigned Depth =3D = cast(Op.getOperand(0))->getZExtValue(); > if (Depth) { > SDValue FrameAddr =3D LowerFRAMEADDR(Op, DAG); > SDValue Offset =3D DAG.getConstant(4, dl, MVT::i32); > return DAG.getLoad(VT, dl, DAG.getEntryNode(), > DAG.getNode(ISD::ADD, dl, VT, FrameAddr, = Offset), > MachinePointerInfo(), false, false, false, 0); > } > . . . > } (PPCTargetLowering::LowerRETURNADDR is similar.)=20 This has a mix of Depth and Offset overall, with the depth going to = LowerFRAMEADDR via Op but Offset used later in GAG.getLoad via adding to = the FrameAddr. This would lead me to guess that the terminology and comments in "case = Builtin::BI__builtin_dwarf_cfa" are wrong and that the = Builder.CreateCall has been given a frame depth, not an offset. > SDValue PPCTargetLowering::LowerFRAMEADDR(SDValue Op, > SelectionDAG &DAG) const { > SDLoc dl(Op); > unsigned Depth =3D = cast(Op.getOperand(0))->getZExtValue(); >=20 > MachineFunction &MF =3D DAG.getMachineFunction(); > MachineFrameInfo *MFI =3D MF.getFrameInfo(); > MFI->setFrameAddressIsTaken(true); >=20 > EVT PtrVT =3D = DAG.getTargetLoweringInfo().getPointerTy(MF.getDataLayout()); > bool isPPC64 =3D PtrVT =3D=3D MVT::i64; >=20 > // Naked functions never have a frame pointer, and so we use r1. For = all > // other functions, this decision must be delayed until during PEI. > unsigned FrameReg; > if (MF.getFunction()->hasFnAttribute(Attribute::Naked)) > FrameReg =3D isPPC64 ? PPC::X1 : PPC::R1; > else > FrameReg =3D isPPC64 ? PPC::FP8 : PPC::FP; >=20 > SDValue FrameAddr =3D DAG.getCopyFromReg(DAG.getEntryNode(), dl, = FrameReg, > PtrVT); > while (Depth--) > FrameAddr =3D DAG.getLoad(Op.getValueType(), dl, = DAG.getEntryNode(), > FrameAddr, MachinePointerInfo(), false, = false, > false, 0); > return FrameAddr; > }=20 Again Op is called Depth --and is used to get from one frame pointer = value to the next: a frame depth. To match g++ 4.2.1 the value to use is 1 for depth. Overall, at least applied to powerpc/powerpc64: > . . . > // TODO: If there's a satisfactory reason, add a target hook for > // this instead of hard-coding 0, which is correct for most = targets. > int32_t Offset =3D 0; I think the comments in this area are actually talking about byte = offsets, not depths and are just wrong. A byte offset of 0 would make = sense relative to hardcoding but the value is actually a frame depth --a = very different context. I think that the naming of the variable is just wrong: it should be = called Depth. And I think that the comments should have originally talked about using = a hard coded Depth 1 to match g++ and preexisting library usage of = __builtin_dwarf_cfa() for C++ and other exception handling (.eh_frame = usage). ANd the code should avhe matched. As far as I can tell this error in the "case = Builtin::BI__builtin_dwarf_cfa:" design was not caught until now. But since the mess has been around a longtime just switching everything = to match the g++ context now likely has its own problems. (Not just a = FreeBSD issue.) For FreeBSD I expect that Depth 1 could be used for powerpc and = powerpc64: if it has been wrong for a long time (not just 3.8.0) then = powerpc/powerpc64 FreeBSD has likely been broken for C++ exception = handling when buildworld was via clang for just as long. (But only = recently has clang gotten this near working for buildworld for at least = one of powerpc/powerpc64. Currently powerpc is closer, given that = powerpc64 does not support softfloat last I knew.) For other TARGET_ARCH's: For FreeBSD armv6 it is less clear to me: it is based on clang as it is = and I do not know what C++ exception ABI it uses. If a modern gcc/g++ = buildworld had problems with C++ exception handling, does anything need = to be done about it? For FreeBSD armv6 and the like: is xtoolchain like = support important? FreeBSD may have similar questions for other TARGET_ARCH's. =3D=3D=3D Mark Millard markmi at dsl-only.net On 2016-Feb-28, at 2:46 AM, Mark Millard wrote: On 2016-Feb-28, at 12:39 AM, Roman Divacky = wrote: >=20 > Mark, >=20 > __builtin_dwarf_cfa() is lowered in clang to llvm intrinsic = eh_dwarf_cfa. > There's a depth argument (which defaults to 0, saying it's correct for = most > targets).=20 >=20 > Then the intrinsic gets lowered in SelectionDAG using > PPCTargetLowering::LowerFRAMEADDR() >=20 >=20 > Can you check that 1) the depth should be 0 for ppc64/ppc32 2) that > LowerFRAMEADDR() does something sensible? >=20 > There's a loop depth-times, so I wonder if that makes a difference. >=20 > Thanks, Roman "Lowered"? I'm not familiar with the clang code base or its terminology. = Handed off to a lower level interface, may be? As near as I can tell libgcc_s could be made to deal with clang 3.8.0's = way of __builtin_dwarf_cfa() working for powerpc/powerpc64. But then use = with g++ would be broken on powerpc/powerpc64 unless there were some = sort of live "which compiler's type of code" test also involved. Having only one libgcc_s and multiple compilers using it for a given = TARGET_ARCH=3D (for example, devel/powerpc64-xtoolchain-gcc like uses) = suggests sticking to one convention per TARGET_ARCH=3D for = __builtin_dwarf_cfa(). I would guess that g++ conventions win in this type of context for = FreeBSD, under the guideline of trying to be gcc 4.2.1 "ABI" compatible. = libgcc_s from FreeBSD works for C++ exceptions with its gcc 4.2.1 for = powerpc and powerpc64 as things are as far as I know. But for clang++ FreeBSD is one context among many and other libraries = may be based on clang 3.8.0's existing interpretation, without gcc/g++ = compatibility constraints. (I've no experience with earlier clang = vintages for the issue.) It may be that any change needs to be FreeBSD = target-triple specific for all I know. In essence: making the convention = part of the ABI chosen. I'll probably get some sleep before looking much at the code that you = reference. A quick look at part of it suggests a fair amount of = research/study for me to interpret things reliably. The loop may be somewhat analogous to _Unwind_RaiseException's loop, but = for a specific depth. I would currently guess that depth 1 would match = gcc 4.2.1's result for __builtin_dwarf_cfa(). But there was also some other "address"(?) builtin support routine = nearby that seemed to call into LowerFRAMEADDR() and I've no clue if g++ = 4.2.1 uses the same depth-figure standard for both sorts of things or = not. For all I know both types of builtins(?) might have mismatches with = gcc/g++ 4.2.1 and both might need fixes. I do vaguely remember seeing a builtin in FreeBSD code for something = that had an explicit number in the argument list, possibly = __builtin_frame_address(n)(?). But I only saw __builtin_dwarf_cfa() with = no argument in the argument list as far as I remember. If clang 3.8.0 and gcc 4.2.1 disagreed about what the numbering standard = referred to for __builtin_frame_address(n) (or whatever it was), that = would not be good and compatibility would imply conversions between the = conventions for the 2 FreeBSD contexts. I have not checked for armv6 related clang++ vs. g++ compatibility for = C++ exception-handling. If anything is not operating for that context I = expect that it would be g++ that generated buildworld code that did not = work based on the FreeBSD source code: clang/clang++ is the normal = compiler and kyua seemed to operate, unlike on the powerpc/powerpc64. I've never tried to build armv6 via an equivalent of = devel/powerpc64-gcc. I do not know if armv6 even uses the same sort of = C++ exception-handling ABI or not. But I do know that = __builtin_dwarf_cfa() is not compatible between clang++ and g++ from the = 2-line source and comparing objdump -d results. So more than powerpc/powerpc64 might be involved overall. =3D=3D=3D Mark Millard markmi at dsl-only.net