From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22b; helo=mail-it0-x22b.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22b.google.com (mail-it0-x22b.google.com [IPv6:2607:f8b0:4001:c0b::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D8034221A2B82 for ; Thu, 14 Dec 2017 06:00:35 -0800 (PST) Received: by mail-it0-x22b.google.com with SMTP id f190so11291259ita.5 for ; Thu, 14 Dec 2017 06:05:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qUfLn6+VfQvSuQdLQtr2XtAoJf9dB7/deQww4jmmx1g=; b=YSbu5el8n0u0S+NgGSPFY4Ub5KUztwdKgcnduxmYM4ui6LOcFQEYM+nPum0igUGPv5 tecPaFj1bFEj5r9CpsmWP+ceB2ovkagiRc15RZRDXMfgyxgfST7gpsec9yMz8GDvkSw6 u7DjX49XlIZbpp4CkvMJBzCOlDSQHKm/xCeBM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qUfLn6+VfQvSuQdLQtr2XtAoJf9dB7/deQww4jmmx1g=; b=RyfxbuNFx3jiadxOnRTO0OQ58uWIHqPk2f5a+flhjE5XiYZGkZUJXc7yiOYggyeFA3 BLi72Vc0KjZUYeZ2tTjpqo5BlM87Sp5rHQWgoPfbibTdveDvze2ksGcfz9KQGmMtZpVS b5O/L+o5Tcl8pKLcytPSWFTHed/G96JG2nHTVEnn4wv0yhExwkIwjNGFHPkjoa5UdLQy l5lr/sAJ9sNrj4VYEUJWGMFpwPPO1HH6vm7ffrcMlktSMGo6PcJczExr82epaQdZ2JFe 5Mn87bBAS64Fpwf76yVvFOZ4N49N8JE/GHw+xBHda8CdOr2tOspsfXF4Zwwc9P5Csfvx DL0w== X-Gm-Message-State: AKGB3mJl6rAZ010JsHABR2ymyOB+k1/knDmCOcHcNPFEmgNJzn2H0j6+ Vs82SLh2FcaYcZ92BhHS7DM7RbKwUAPWW/aVsbV8vw== X-Google-Smtp-Source: ACJfBos3Fkpa9ACfdKV53JKc/mzBArT5rVAsbEjTM4YgrND6A2/GtkxSG2c0T2OjYLEjqZDnlEPT+/w7+k5Lz/vYc3U= X-Received: by 10.36.71.83 with SMTP id t80mr3233411itb.48.1513260315838; Thu, 14 Dec 2017 06:05:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.52.2 with HTTP; Thu, 14 Dec 2017 06:05:14 -0800 (PST) In-Reply-To: References: <20171208140631.4252-1-pete@akeo.ie> <20171208140631.4252-5-pete@akeo.ie> From: Ard Biesheuvel Date: Thu, 14 Dec 2017 15:05:14 +0100 Message-ID: To: Pete Batard Cc: "edk2-devel@lists.01.org" , "Gao, Liming" Subject: Re: [PATCH v3 4/6] ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Dec 2017 14:00:36 -0000 Content-Type: text/plain; charset="UTF-8" On 13 December 2017 at 14:56, Pete Batard wrote: > Hi Ard. Thanks for reviewing this. > > On 2017.12.12 19:59, Ard Biesheuvel wrote: >>> >>> + EXPORT _fltused >>> + EXPORT __brkdiv0 >>> + >> >> >> Why are these being exported? Are these part of the CRT ABI? > > > Good point. I exported them because I was encountering undefined symbols for > those when I started working on adding ARM support. But I now suspect they > may have come from an export statement from VS headers (which we no longer > include), as re-test shows that the exports seem to be superfluous. > > I will test further to confirm that this is the case, and remove the > unneeded exports. > >>> +__brkdiv0 >>> + udf #249 >>> + >> >> >> This will cause a crash when invoked, in a way that gives very little >> to go on to figure out what happens. > > > Yes. I mostly copied what ReactOS was doing here, since it should be similar > to what Microsoft does internally. > >> Perhaps branch to a C function >> here that calls ASSERT() and CpuDeadLoop ? > > > I don't mind giving it a try, if we keep the separate .asm files for MSVC. > > On the other hand, if I am to try to reuse existing assembly, as you suggest > below, then I don't think I want to introduce anything that will differ from > how divisions by zero are currently handled there. As far as I can tell (for > RVCT, which is the only assembly that might be suitable for reuse with MSFT) > the current way of handling a zero divisor is to set result and remainder to > 0 and return (as per uldiv.asm). For gcc, we also have the following comment > in div.S: > > "@ What to do about division by zero? For now, just return." > > So, while I agree that calling ASSERT and CpuDeadLoop is probably the better > approach, I'd rather have the introduction of this behaviour be the subject > of a separate ARM harmonization patch, that applies to all the toolchains, > rather than something that's specific to Visual Studio usage. > I agree. Reusing code is more important than having one flavour of division that will spot div-by-0. But we really don't want the UDF, because it will crash unconditionally without a hint of what happened. >> There are many different division implementations already in the same >> directory. Do we really need a new one? > > > I'll look into that. My goal with this series was to make sure that the > changes being introduced would not break existing toolchains, which is why I > saw it preferable to keep the MSVC intrinsics separate. > > Now, gcc assembly is not something that can be reused easily with the MS > toolchain, so the only possibility are the RVCT .asm files. > > There's a fair bit of work involved into trying to figure out if and how we > can reuse that code, but I'll see what I can do. It'll probably be a few > weeks before I get back to this list on that however. > I'm prepared to be pragmatic here. I would prefer to have all platforms run the same low level division code, but I understand that it is tightly coupled to the compiler, given that they are intrinsics. So yes, please try to reuse the RVCT code if feasible without a ton of rework, but it seems like we will need to live with some MSFT specific hooks anyway (if I understand the prototypes correctly)