r225134 - Fix default image name to 'a.exe' on Windows, instead 'a.out'.

[Hans Wennborg]

On Fri, Jan 9, 2015 at 12:16 AM, Yaron Keren <yaron.keren at gmail.com> wrote:
> Yes, it's a mess.
> This patch looks good to me.

Thanks! I added a test and committed in r225530.


> 2015-01-09 1:24 GMT+02:00 Hans Wennborg <hans at chromium.org>:
>>
>> Hmm, the whole thing seems a bit of a mess :/
>>
>> However, I don't think we need to base DefaultImageName on the full
>> logic of computeTargetTriple(), which is mostly fine-tuning the triple
>> based on various flags. I think it's enough to make DefaultImageName
>> match DefaultTargetTriple, like in the attached patch. What do you
>> think?
>>
>> On Thu, Jan 8, 2015 at 2:38 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
>> > Rethinking, my suggestion is not good. getToolChain is called several
>> > times
>> > so returning the target triple and setting DefaultImageName baed on it
>> > duplicates the same operation several times.
>> >
>> > Moving DefaultImageName to llvm::Triple or adding the helper function
>> > will
>> > not help, as the the root of the problem is that the target triple is
>> > not
>> > known when it's needed: It is computed in the const getToolChain but not
>> > stored.
>> > Storing in Driver::TargetTriple would make getToolChain non-const or or
>> > Driver::TargetTriple mutuable just the same as DefaultImageName, which
>> > isn't
>> > cleaner.
>> >
>> > Also, look at the FIXME at line 339 which seems to workaround the same
>> > const
>> > issue. Maybe we can solve both.
>> >
>> > What do you think?
>> >
>> >
>> >
>> > 2015-01-08 23:59 GMT+02:00 Yaron Keren <yaron.keren at gmail.com>:
>> >>
>> >> The Driver needs DefaultImageName which is based on the target triple
>> >> but
>> >> the only location the target triple is computed using
>> >> computeTargetTriple()
>> >> is in getToolChain() and the target triple is not kept otherwise.
>> >>
>> >> We could have getToolChain return the computed target triple to its
>> >> caller
>> >> BuildCompilation (non const mmeber function) and have BuildCompilation
>> >> set
>> >> DefaultImageName based on the OS. Does this make sense to you?
>> >>
>> >> I'll fix the test.
>> >>
>> >>
>> >>
>> >>
>> >>
>> >>
>> >> 2015-01-08 23:16 GMT+02:00 Hans Wennborg <hans at chromium.org>:
>> >>>
>> >>> On Sun, Jan 4, 2015 at 5:48 AM, Yaron Keren <yaron.keren at gmail.com>
>> >>> wrote:
>> >>> > Author: yrnkrn
>> >>> > Date: Sun Jan  4 07:48:30 2015
>> >>> > New Revision: 225134
>> >>> >
>> >>> > URL: http://llvm.org/viewvc/llvm-project?rev=225134&view=rev
>> >>> > Log:
>> >>> > Fix default image name to 'a.exe' on Windows, instead 'a.out'.
>> >>> > This applies to mingw as clang-cl already has its own logic for the
>> >>> > filename.
>> >>>
>> >>> [..]
>> >>>
>> >>> > Modified: cfe/trunk/lib/Driver/Driver.cpp
>> >>> > URL:
>> >>> >
>> >>> > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=225134&r1=225133&r2=225134&view=diff
>> >>> >
>> >>> >
>> >>> > ==============================================================================
>> >>> > --- cfe/trunk/lib/Driver/Driver.cpp (original)
>> >>> > +++ cfe/trunk/lib/Driver/Driver.cpp Sun Jan  4 07:48:30 2015
>> >>> > @@ -1996,6 +1996,8 @@ const ToolChain &Driver::getToolChain(co
>> >>> >                                        StringRef DarwinArchName)
>> >>> > const
>> >>> > {
>> >>> >    llvm::Triple Target = computeTargetTriple(DefaultTargetTriple,
>> >>> > Args,
>> >>> >                                              DarwinArchName);
>> >>> > +  if (Target.isOSWindows())
>> >>> > +    DefaultImageName = "a.exe";
>> >>>
>> >>> This doesn't seem right. Why should getToolChain() be changing the
>> >>> Driver's DefaultImageName? That this requires DefaultImageName to be
>> >>> mutable makes it feel even more iffy.
>> >>>
>> >>> Maybe DefaultImageName should be a property of the Target? Or perhaps
>> >>> there should be a "const char *Driver::DefaultImageName(llvm::Triple)"
>> >>> method?
>> >>>
>> >>>
>> >>> > Modified: cfe/trunk/test/Driver/lto.c
>> >>>
>> >>> It would be nice to test for the default image name explicitly by
>> >>> setting -target and looking at the cc1 invocation.
>> >>>
>> >>>  - Hans
>> >>
>> >>
>> >
>
>