[PATCH] Improve Windows toolchain support for non-standard environments.

[Aaron Ballman]

On Fri, Oct 17, 2014 at 4:49 PM, Zachary Turner <zturner at google.com> wrote:
> In that case, can this be a follow-up patch (which may not happen
> immediately)?  It's using the A functions before my patch anyway, so I'm
> introducing anything that's worse than before.  I can leave a comment in the
> code that mentions that we need to eventually convert to using W functions,
> and doing so requires refactoring the registry support code.

I think that's reasonable -- this is still a step forward. However, it
would be good to open up a bug against this. Paths are configurable
for the SDK and toolchain installations, so it's perfectly reasonable
for a user to have these installed on a path with Unicode characters.
In those cases, we will fail hard.

~Aaron

>
> On Fri, Oct 17, 2014 at 1:43 PM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> On Fri, Oct 17, 2014 at 4:38 PM, Zachary Turner <zturner at google.com>
>> wrote:
>> >
>> >
>> > On Fri, Oct 17, 2014 at 1:27 PM, Aaron Ballman <aaron.ballman at gmail.com>
>> > wrote:
>> >>
>> >> On Fri, Oct 17, 2014 at 4:22 PM, Zachary Turner <zturner at google.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Fri, Oct 17, 2014 at 12:48 PM, Aaron Ballman
>> >> > <aaron.ballman at gmail.com>
>> >> > wrote:
>> >> >>
>> >> >> On Fri, Oct 17, 2014 at 3:06 PM, Zachary Turner <zturner at google.com>
>> >> >> wrote:
>> >> >> > -
>> >> >> >  std::unique_ptr<Command> visualstudio::Compile::GetCommand(
>> >> >> >      Compilation &C, const JobAction &JA, const InputInfo &Output,
>> >> >> >      const InputInfoList &Inputs, const ArgList &Args,
>> >> >> > Index: lib/Driver/WindowsToolChain.cpp
>> >> >> >
>> >> >> > ===================================================================
>> >> >> > --- lib/Driver/WindowsToolChain.cpp
>> >> >> > +++ lib/Driver/WindowsToolChain.cpp
>> >> >> > @@ -77,61 +77,59 @@
>> >> >> >    return getArch() == llvm::Triple::x86_64;
>> >> >> >  }
>> >> >> >
>> >> >> > +#ifdef USE_WIN32
>> >> >> > +static bool readFullStringValue(HKEY hkey, const char *valueName,
>> >> >> > +                                std::string &value) {
>> >> >>
>> >> >> We should be preferring the W versions of these APIs instead of the
>> >> >> A
>> >> >> versions, especially since this is being used to pull out file
>> >> >> paths.
>> >> >
>> >> > How does this work, since ultimately all of clang uses non-wide
>> >> > character
>> >> > strings anyway.  I mean I know how to convert between the two, but I
>> >> > was
>> >> > under the impression that everything was just already broken because
>> >> > afaik
>> >> > we don't ever use W functions anywhere else.
>> >>
>> >> My understanding is that we use the W versions of the APIs and
>> >> immediately convert to UTF-8 to store internally. When we require
>> >> interaction in the other direction, we convert back to UTF-16. At
>> >> least, this is how we work with things like command line arguments and
>> >> files. As an example, see Process::GetArgumentVector.
>> >
>> >
>> > So llvm::sys::windows::UTF8ToUTF16 and its counterpart are not exposed
>> > in a
>> > public header.  Is there an accepted way to re-use them here, or do I
>> > need
>> > to duplicate the code in clang?
>>
>> There's not an accepted way currently, but duplication isn't the answer
>> either.
>>
>> I think the registry code should be pulled down into an LLVM Support
>> interface that's available on Windows (since we're already using
>> USE_WIN32 within clang, I don't think the interface needs x-platform
>> stubs), and the conversion routines hoisted to a place where they can
>> be exposed for use within Support so that they can be used with the
>> registry code. Registry use shouldn't be overly widespread, so I think
>> the abstraction could be fairly simplistic.
>>
>> ~Aaron
>
>