<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Nov 12, 2018 at 4:58 AM Hans Wennborg <<a href="mailto:hans@chromium.org">hans@chromium.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hmm, maybe I misunderstood your initial request for this.<br>
<br>
The current implementation does what the warnings says: If the<br>
compiler falls back to cl.exe, /Zc:dllexportInlines- will be ignored.<br>
This suggests to the user that it's a bad idea, but they can go ahead<br>
if they want to.<br>
<br>
It sounds like you're suggesting we should always ignore<br>
/Zc:dllexportInlines- when used together with the /fallback flag?</blockquote><div><br></div><div>I thought we'd always emit an error if both flags are present, actually, something like "/dllexportInlines- is ABI-changing and not compatible with /fallback". If you still need /fallback, you can't really use /dllexportInlines-.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I'm<br>
not sure that's necessarily better.. it depends on why they're using<br>
/fallback I guess. If it's because the user has one file that clang-cl<br>
can't handle for some reason, but they still want<br>
/Zc:dllexportInlines- for the rest, then ignoring it would break that.<br>
<br>
In general, I'm not sure how widely used /fallback is anymore. I<br>
wonder if it would be possible to remove it...<br>
<br>
On Fri, Nov 9, 2018 at 9:52 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
> Ah cool. But the diagnostic is "option /dllexportInlines- is ignored when<br>
> /fallback happens" – shouldn't it be ignored regardless of if fallback<br>
> happens? Does that happen and the warning text is wrong?<br>
><br>
> On Fri, Nov 9, 2018 at 11:20 AM Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
>><br>
>> On Fri, Nov 9, 2018 at 4:53 PM, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
>> > This only prints the warning when /fallback actually happens, right?<br>
>><br>
>> No, it prints it when the fallback job is created, not when (or if) it<br>
>> runs. I.e. it prints if the /fallback flag is used, regardless of<br>
>> whether it actually falls back or not. This is reflected by the test<br>
>> which uses -###, i.e. nothing is getting run.<br>
>><br>
>> So I think we're good :-)<br>
>><br>
>> > I don't<br>
>> > think that's good enough: If we build a few TUs with /dllexportInlines-<br>
>> > and<br>
>> > don't fall back, those .obj files are not abi compatible with the<br>
>> > cl-built<br>
>> > ones. (Consider all dllexport TUs of a header being built with clang-cl<br>
>> > but<br>
>> > all dllimport versions of the same header being built by cl – I think<br>
>> > this<br>
>> > will cause link errors). SO I think we should error out if<br>
>> > /dllexportIlnlines- /fallback is on the same line, even if the fallback<br>
>> > doesn't actually happen.<br>
>> ><br>
>> > On Fri, Nov 9, 2018 at 8:28 AM Takuto Ikuta via cfe-commits<br>
>> > <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> Author: tikuta<br>
>> >> Date: Fri Nov 9 05:25:45 2018<br>
>> >> New Revision: 346491<br>
>> >><br>
>> >> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=346491&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=346491&view=rev</a><br>
>> >> Log:<br>
>> >> [clang-cl] Add warning for /Zc:dllexportInlines- when the flag is used<br>
>> >> with /fallback<br>
>> >><br>
>> >> Summary:<br>
>> >> This is followup of<br>
>> >> <a href="https://reviews.llvm.org/D51340" rel="noreferrer" target="_blank">https://reviews.llvm.org/D51340</a><br>
>> >><br>
>> >> Reviewers: hans, thakis<br>
>> >><br>
>> >> Reviewed By: hans<br>
>> >><br>
>> >> Subscribers: cfe-commits, llvm-commits<br>
>> >><br>
>> >> Differential Revision: <a href="https://reviews.llvm.org/D54298" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54298</a><br>
>> >><br>
>> >> Modified:<br>
>> >> cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td<br>
>> >> cfe/trunk/lib/Driver/ToolChains/MSVC.cpp<br>
>> >> cfe/trunk/test/Driver/cl-options.c<br>
>> >><br>
>> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td?rev=346491&r1=346490&r2=346491&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td?rev=346491&r1=346490&r2=346491&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td (original)<br>
>> >> +++ cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td Fri Nov 9<br>
>> >> 05:25:45 2018<br>
>> >> @@ -161,6 +161,10 @@ def warn_drv_yc_multiple_inputs_clang_cl<br>
>> >> "support for '/Yc' with more than one source file not implemented<br>
>> >> yet;<br>
>> >> flag ignored">,<br>
>> >> InGroup<ClangClPch>;<br>
>> >><br>
>> >> +def warn_drv_non_fallback_argument_clang_cl : Warning<<br>
>> >> + "option '%0' is ignored when /fallback happens">,<br>
>> >> + InGroup<OptionIgnored>;<br>
>> >> +<br>
>> >> def err_drv_invalid_value : Error<"invalid value '%1' in '%0'">;<br>
>> >> def err_drv_invalid_int_value : Error<"invalid integral value '%1' in<br>
>> >> '%0'">;<br>
>> >> def err_drv_invalid_remap_file : Error<<br>
>> >><br>
>> >> Modified: cfe/trunk/lib/Driver/ToolChains/MSVC.cpp<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/MSVC.cpp?rev=346491&r1=346490&r2=346491&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/MSVC.cpp?rev=346491&r1=346490&r2=346491&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- cfe/trunk/lib/Driver/ToolChains/MSVC.cpp (original)<br>
>> >> +++ cfe/trunk/lib/Driver/ToolChains/MSVC.cpp Fri Nov 9 05:25:45 2018<br>
>> >> @@ -669,6 +669,12 @@ std::unique_ptr<Command> visualstudio::C<br>
>> >> // them too.<br>
>> >> Args.AddAllArgs(CmdArgs, options::OPT_UNKNOWN);<br>
>> >><br>
>> >> + // Warning for ignored flag.<br>
>> >> + if (const Arg *dllexportInlines =<br>
>> >> + Args.getLastArg(options::OPT__SLASH_Zc_dllexportInlines_))<br>
>> >> +<br>
>> >><br>
>> >> C.getDriver().Diag(clang::diag::warn_drv_non_fallback_argument_clang_cl)<br>
>> >> + << dllexportInlines->getAsString(Args);<br>
>> >> +<br>
>> >> // Input filename.<br>
>> >> assert(Inputs.size() == 1);<br>
>> >> const InputInfo &II = Inputs[0];<br>
>> >><br>
>> >> Modified: cfe/trunk/test/Driver/cl-options.c<br>
>> >> URL:<br>
>> >><br>
>> >> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=346491&r1=346490&r2=346491&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=346491&r1=346490&r2=346491&view=diff</a><br>
>> >><br>
>> >><br>
>> >> ==============================================================================<br>
>> >> --- cfe/trunk/test/Driver/cl-options.c (original)<br>
>> >> +++ cfe/trunk/test/Driver/cl-options.c Fri Nov 9 05:25:45 2018<br>
>> >> @@ -494,6 +494,8 @@<br>
>> >> // NoDllExportInlines: "-fno-dllexport-inlines"<br>
>> >> // RUN: %clang_cl /Zc:dllexportInlines /c -### -- %s 2>&1 | FileCheck<br>
>> >> -check-prefix=DllExportInlines %s<br>
>> >> // DllExportInlines-NOT: "-fno-dllexport-inlines"<br>
>> >> +// RUN: %clang_cl /fallback /Zc:dllexportInlines- /c -### -- %s 2>&1 |<br>
>> >> FileCheck -check-prefix=DllExportInlinesFallback %s<br>
>> >> +// DllExportInlinesFallback: warning: option '/Zc:dllexportInlines-'<br>
>> >> is<br>
>> >> ignored when /fallback happens [-Woption-ignored]<br>
>> >><br>
>> >> // RUN: %clang_cl /Zi /c -### -- %s 2>&1 | FileCheck -check-prefix=Zi<br>
>> >> %s<br>
>> >> // Zi: "-gcodeview"<br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> cfe-commits mailing list<br>
>> >> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>