This revision was automatically updated to reflect the committed changes.
Closed by commit rL298098: [clang-cl] Fix cross-compilation with MSVC 2017.
(authored by zturner).
Changed prior to commit:
https://reviews.llvm.org/D30991?vs=92050&id=92154#toc
Repository:
rL LLVM
https://reviews.llv
hamzasood added a comment.
In https://reviews.llvm.org/D30991#703199, @zturner wrote:
> lgtm, do you have commit access?
No, I don't.
https://reviews.llvm.org/D30991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cg
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
lgtm, do you have commit access?
https://reviews.llvm.org/D30991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/
hamzasood updated this revision to Diff 92050.
hamzasood added a comment.
@zturner: "it's just a vector of pointers"
Oh.. I was so focussed on reducing copies that I completely forgot it's
just an array of pointers being copied, not the entire environment block
contents. In that case, yeah.
zturner added a comment.
Looks good with one more suggested fix.
Comment at: include/clang/Driver/Job.h:129
+ /// the given vector is to be copied in as opposed to moved.
+ void setEnvironment(const std::vector &NewEnvironment);
+
Since it's just a vector of
thakis added a comment.
Ah ok, thanks for explaining. In that case, this sounds fine and I'll leave the
review to zturner.
https://reviews.llvm.org/D30991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman
hamzasood added a comment.
In https://reviews.llvm.org/D30991#702965, @thakis wrote:
> When you say "cross-compiling", you mean targeting Windows while running on
> non-Windows, right? How do dlls get loaded there at all?
>
> Also, when does clang invoke link.exe? Normally on Windows the linker
zturner added a comment.
In https://reviews.llvm.org/D30991#702966, @zturner wrote:
> In https://reviews.llvm.org/D30991#702965, @thakis wrote:
>
> > When you say "cross-compiling", you mean targeting Windows while running on
> > non-Windows, right? How do dlls get loaded there at all?
> >
> > A
zturner added a comment.
In https://reviews.llvm.org/D30991#702965, @thakis wrote:
> When you say "cross-compiling", you mean targeting Windows while running on
> non-Windows, right? How do dlls get loaded there at all?
>
> Also, when does clang invoke link.exe? Normally on Windows the linker is
thakis added a comment.
When you say "cross-compiling", you mean targeting Windows while running on
non-Windows, right? How do dlls get loaded there at all?
Also, when does clang invoke link.exe? Normally on Windows the linker is
invoked directly, no through the compiler driver. Are you using c
hamzasood added a comment.
*dynamic loader
And that's the cross-compiling link.exes that have those requirements. The
native ones have the required dlls in their containing directories.
https://reviews.llvm.org/D30991
___
cfe-commits mailing list
c
hamzasood added a comment.
In https://reviews.llvm.org/D30991#702941, @thakis wrote:
> What env vars are needed here? Reading an env file seems a bit inelegant,
> could we pass the values of these env vars as flags instead?
>
> For example, MSVC2015 needs %INCLUDE%, but for cross-compiling (and
thakis added a comment.
What env vars are needed here? Reading an env file seems a bit inelegant, could
we pass the values of these env vars as flags instead?
For example, MSVC2015 needs %INCLUDE%, but for cross-compiling (and for
building on Windows without requiring env vars to be set we adde
hamzasood updated this revision to Diff 92019.
hamzasood added a comment.
- Added an assertion in Command::Execute to ensure the environment vector is
null-terminated before using it.
- Split Command::setEnvironment so that there's an overload to specifically
handle the case where the input vect
hamzasood added inline comments.
Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+ if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(EnvBlockWide.get()),
+ EnvBlockLen * sizeof(EnvBlockWide[0])),
+
hamzasood marked an inline comment as done.
hamzasood added inline comments.
Comment at: lib/Driver/ToolChains/MSVC.cpp:487-488
+ if (!llvm::convertUTF16ToUTF8String(
+ llvm::ArrayRef(reinterpret_cast(EnvBlockWide.get()),
+ EnvBlo
zturner added inline comments.
Comment at: lib/Driver/ToolChains/MSVC.cpp:517
+ }
+}
+ SkipSettingEnvironment:
zturner wrote:
> I think you need to push 1 more null terminator onto the end here to
> terminate the block.
Actually if you use the `std::ti
zturner added inline comments.
Comment at: lib/Driver/Job.cpp:312
SmallVector Argv;
+ auto Envp = Environment.size() > 1
+? const_cast(Environment.data())
Can you add `assert(Environment.back() == nullptr);`?
Comment at: lib
hamzasood updated this revision to Diff 91962.
hamzasood added a comment.
Changed the call to GetEnvironmentStrings to use the unicode version
https://reviews.llvm.org/D30991
Files:
include/clang/Driver/Job.h
lib/Driver/Job.cpp
lib/Driver/ToolChains/MSVC.cpp
Index: lib/Driver/ToolChains/
zturner requested changes to this revision.
zturner added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Driver/ToolChains/MSVC.cpp:48-50
+ // Undefine this macro so we can call the ANSI version of the function.
+ #undef GetEnvironmentStrings
+
hamzasood created this revision.
- Adds a settable environment to clang::driver::Command.
- Fixes cross compiling with Visual Studio 2017 by ensuring the linker has the
correct environment.
This was originally part of https://reviews.llvm.org/D28365, however it was
removed for separate review a
21 matches
Mail list logo
