In D147212#4234863, @efriedma wrote:

In D147212#4234854, @mstorsjo wrote:

I'm assuming the load/store ops are getting generated by expanding a PUSH/POP, and not some other transform? (So the offset will actually reliably be 4, in practice?)

Not sure actually - if looking at the output of -print-after-all here, the t2STR_PRE appears as such directly, not as a transformation of something else - I don't directly see where it comes from (other than printf instrumenting the code in Target/ARM to see which reference to t2STR_PRE gets hit).

Skimming the code, it looks like it's coming out of ARMFrameLowering::emitPushInst... which should be safe in that respect.

I'm still not entirely a fan of the brittle test that tries to link in two different ways though, but I guess it can be tolerable if it's only this one test.

LGTM, thanks!

In D147212#4234136, @efriedma wrote:

Can you stick the testcase into wineh-opcodes.ll?

Overall LGTM, but I think it maybe would be nicer to split out the long->intptr_t changes into a separate patch.

Ping

Ping @phosek

Looks reasonable overall I think, nothing to object to from my point of view, just a few minor comments.. The patch is a bit unwieldy with the number of tests marked as unsupported, but I guess it's as good as it will be. If we're only touching tests that use specifically clang_cl_* to build/run, the mass editing probably is ok.

LGTM overall.

I tested this, and this does fix the repro from the linked issue.

Btw, for issues like these, it would be super useful if there was a CI configuration that builds libcxx/libunwind/libcxxabi from scratch in an environment where there's no preexisting unwinder or C++ library.

Thanks for the revert - I was also just looking into a build issue that seems to be caused by this commit.

In D146848#4221771, @mstorsjo wrote:

In D146848#4221240, @mati865 wrote:

Oh, that's unfortunate.
Maybe llvm-windres should generate a warning in such case?

Hmm, maybe... (I'd need to think about it again to sort my thoughts out about it; I had written this patch 6 months ago but never took the time to finish it to have it reviewed.)

Superseded by the improved version of D146894.

LGTM, thanks! This is indeed the neatest solution so far!

In D146925#4225492, @vadikp-intel wrote:

I will be good with this solution, but I think there could be a simpler one in the end. While CMAKE does wants to see a source, that does not have to be a cpp (which is what's causing this need to unnecessarily feed the first step's library to satisfy lib's attempt at def symbols resolution). There are other source types (e.g., a. txt) which will satisfy the source presence requirement but which CMAKE will filter out from the librarian generating the right command line for it.

In D144419#4221935, @vadikp-intel wrote:

The source of problem is the attempt to re-use the results of the first build in order to avoid rebuilding the sources the second time to just re-generate the import library. This is being done by feeding the main build's import library as a source to the second one (@301) with CMAKE ending up assuming the library is located where the sources are, hence the reference to projects/openmp/runtime/src/libomp.dll.lib instead of what should be lib/libomp.dll.lib in the LLVM based build.

In D146398#4221400, @fsb4000 wrote:

Sorry, I didn't have time on the weekdays. But I have time today.

Yes, it looks good.

In D146848#4221240, @mati865 wrote:

Oh, that's unfortunate.
Maybe llvm-windres should generate a warning in such case?

Reordered the checks to check for "clang-<major>" before plain "clang", as suggested in the linked discussion.

In D144419#4219864, @vadikp-intel wrote:

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

In D144419#4219864, @vadikp-intel wrote:

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

In D144419#4219492, @hans wrote:

Also, as mentioned above, the Windows LLVM release currently can't build OpenMP, so as it currently stands LLVM 17 will not include the OpenMP runtime for Windows.

In D146797#4219456, @aganea wrote:

LG.

Just a thought: as future work, we should probably have an integration with LLVM_TOOL_LLVM_DRIVER_BUILD?

In D146764#4219278, @hans wrote:

+mstorsjo is this okay for mingw mode too?

It seems like this change has broken building OpenMP with MSVC as a toplevel project from the llvm-project/llvm directory. To reproduce, make a build directory as a subdirectory to llvm-project/llvm, and configure it with cmake .. -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;openmp". When I try to build with ninja, I get the following error:

ninja: error: 'projects/openmp/runtime/src/libomp.dll.lib', needed by 'lib/libomp.lib', missing and no known rule to make it

It seems like some of the subdirectory path prefixes end up applied inconsistently breaking the dependency chain here, making ninja refuse to proceed.

In D146621#4213136, @alvinhochun wrote:

I did test this patch manually, but I don't have an environment for running the lit test so I may have to ask @mstorsjo to try that. Sorry to bother you :)

@fsb4000 Can you have a look at this, does it seem reasonable to you? (As mentioned in the patch description, I'm not a fan of the amount of ifdefs that this adds, but I didn't see a good way of getting away with fewer of them either.

In D146666#4214857, @smeenai wrote:

In D146666#4214806, @mstorsjo wrote:

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

Yeah, in one sense, that’s the correct thing to do. But on the other hand, even if I’m doing a simple native build, if I’m going out of my way to specify the path to these tools, perhaps we should respect it. We already do it somewhat inconsistently - AFAIK the variables do take effect for the tablegen family of tools without checking the LLVM_USE_HOST_TOOLS variables.

The flip side could be that you have a shared configuration for all your builds that just sets that flag in case it's needed, and that shared config is used by both native and cross-compilation. Not sure how realistic that is either, but I can see someone being surprised the other way as well.

In D146666#4214769, @smeenai wrote:

I'd need to learn more about the use case (I see you're awaiting a response on the issue), but I'm not a huge fan personally; it seems really weird to not be going through the standard CMake mechanisms for cross-compilation (setting a CMAKE_SYSTEM_NAME), and if you do that then this should work as expected. We should understand why LLVM_USE_HOST_TOOLS isn't being set automatically.

FYI, most (all?) use of llvm::Optional has been migrated to std::optional these days.

In D145389#4181206, @mstorsjo wrote:

In D145389#4181188, @mmuetzel wrote:

Is it worth cherry-picking this to LLVM 16?
It seems low-risk. But iiuc, Flang isn't advertised as being production-ready anyway.

Yeah this is very low risk, so it should be a good fit for backporting to the release. But AFAIK they're trying hard to squeeze out the last bugs to get 16.0.0 out now, and this is kinda low priority, so perhaps it's better to not make more noise about this right now - perhaps it's best to wait until after 16.0.0 to file a backport request?

Maybe adjust the commit message a little,

In D146490#4209495, @aganea wrote:

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle for network drives/paths. Any other suggestions?

Rebased on latest git, to get a cleaner CI run.

In D146352#4205131, @chapuni wrote:

@mstorsjo How about it?

• llvm-minimal-tblgen -- Build only in the tree.
• llvm-tblgen includes codegen emitters. It may be exported. It may be used to check-llvm (my patches to test/TableGen would be unneeded)

In D146190#4199844, @Mordante wrote:

I'm not really fond of declaring the MSVC ABI as unstable. If there is no other way, please add this information in the commit message. That way we can find back why it was decided to declare the ABI on a platform as unstable.

Looks reasonable I guess - but I think it would be good to mention the env variables INCLUDE and LIB too, for alternative ways of finding the same things - even if it's not strictly the same as what this new section talks about.

In D145962#4193828, @jacek wrote:

In D145962#4192325, @mstorsjo wrote:

So, MS link.exe fills sections with zero? Is that a specific design choice (i.e. does it fill with 0xcc on x86) or is it simply the case that link.exe doesn't try to fill sections with sentinel instructions at all?

It seems to be a design choice: link.exe does fill with 0xcc on x64. I will add a test case for x64.

Looks reasonable to me, but I don't feel entirely confident about what this does in the common MC layer changes.

Thanks for the patch - I've consiered doing something about this at some time. At the time, I considered filling areas with a more specific pattern, mapping to trapping instructions similar to int3, but since it requires multibyte patterns instead of filling with a single byte, I never got to doing anything about it.

Adding @aaron.ballman as fallback Clang reviewer here. While I did touch code in the vicinity of this area recently, I'm not familiar enough with the whole area to take on reviewing it at the moment.

I took a look at what's going wrong here; I diffed the cmake output from a cmake run before and after this change:

@@ -17,12 +17,12 @@
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS
 -- Performing Test LLVM_RUNTIMES_LINKING_WORKS - Success
 -- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG
--- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Failed
+-- Performing Test CXX_SUPPORTS_NOSTDLIBXX_FLAG - Success
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG
 -- Performing Test CXX_SUPPORTS_NOSTDINCXX_FLAG - Failed
 -- Using Release VC++ CRT: MD
 -- Performing Test SUPPORTS_BREPRO
--- Performing Test SUPPORTS_BREPRO - Success
+-- Performing Test SUPPORTS_BREPRO - Failed
 -- Looking for os_signpost_interval_begin
 -- Looking for os_signpost_interval_begin - not found
 -- Found Python3: /usr/bin/python3.10 (found version "3.10.6") found components: Interpreter
@@ -33,8 +33,6 @@
 -- Using libc++ testing configuration: /home/martin/code/llvm-project/libcxx/test/configs/llvm-libc++-shared-clangcl.cfg.in
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG
 -- Performing Test CXX_SUPPORTS_UNWINDLIB_EQ_NONE_FLAG - Failed
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG
--- Performing Test C_SUPPORTS_NODEFAULTLIBS_FLAG - Failed
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA
 -- Performing Test C_SUPPORTS_COMMENT_LIB_PRAGMA - Failed
 -- Performing Test CXX_SUPPORTS_FALIGNED_ALLOCATION_FLAG
@@ -44,71 +42,65 @@
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG
 -- Performing Test CXX_SUPPORTS_FVISIBILITY_EQ_HIDDEN_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_W4_FLAG
--- Performing Test CXX_SUPPORTS_W4_FLAG - Success
+-- Performing Test CXX_SUPPORTS_W4_FLAG - Failed
 -- Performing Test CXX_SUPPORTS_WEXTRA_FLAG

LGTM

In D145389#4181188, @mmuetzel wrote:

Is it worth cherry-picking this to LLVM 16?
It seems low-risk. But iiuc, Flang isn't advertised as being production-ready anyway.