r346491 - [clang-cl] Add warning for /Zc:dllexportInlines- when the flag is used with /fallback

[Takuto Ikuta (生田拓人) via cfe-commits]

Hmm, I see. Then I think /Zc:dllexportInlines- and /fallback should
not be used  at the same time.
https://reviews.llvm.org/D54426

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