Thanks, this looks more complete and consistent to me now!

In D128268#3614661, @labath wrote:

In D128268#3611053, @mstorsjo wrote:

The odd thing about the second one is the added hardcoded AddArch(ArchSpec("i686-pc-windows")); and AddArch(ArchSpec("i386-pc-windows")); at the top of PlatformWindows.cpp.

Anyway, it does seem to work fine in my quick test to get rid of this duality, so I'll post a patch that does that.

I think those hardcoded lines are there precisely because of the duality (i.e. wanting to support both i386 and i686 regardless of what the host layer reports).

There's consistent typos about the test name in the commit subject and description.

FWIW, llvm/lib/Object/COFFObjectFile.cpp also contains a decodeBase64StringEntry function that probably should be refactored into using this, if we make a more proper toplevel API for it.

I presume this is ok - if the main intent of this test is to test handling of pointers and the size thereof in PDBs.

In D128600#3611520, @alvinhochun wrote:

In D128600#3611043, @mstorsjo wrote:

I guess the more complete/proper way forward would make a mingw-w64 specific implementation of these (or some similar) functions too, if __USE_MINGW_ANSI_STDIO is set to 1. (No idea offhand how hard that would be though...)

Hypothetically, if this means implementing custom formatting routines, wouldn't it be better to just use the same implementation for both msvc and mingw-w64?

In D128268#3608197, @labath wrote:

In D128268#3604555, @mstorsjo wrote:

I found that this duality was introduced in 5e6f45201f0b62c1e7a24fc396f3ea6e10dc880d / D7120 and ad587ae4ca143d388c0ec4ef2faa1b5eddedbf67 / D4658 (CC @zturner), what do you make out of the reasonings in those commits?

The first patch seems like it's just working around some mismatches in the windows dynamic loader plugin. I think a better approach would be to have the dynamic loader claim both architectures, though I don't think that is necessary if we're just consistent about what we use. I don't see anything wrong with the second patch (the darwin platform does something similar for arm architectures, even though I'm not convinced it's necessary (the reason it's necessary for darwin is because there they actually make a distinction between armv6XX and armv7YY and treat those as different architectures).

In D128600#3610680, @alvinhochun wrote:

long double is an issue though...

In D128410#3608190, @labath wrote:

In D128410#3604927, @alvinhochun wrote:

It may be possible to stuff a pointer to an EXCEPTION_RECORD into another EXCEPTION_RECORD and use RtlRaiseException to generate the exception, but you'll have to test how it actually works.

That would be pretty cool.

In D128268#3604081, @labath wrote:

If we'd just set this to the baseline, i386, would that have any effect for how lldb e.g. is able to disassemble/interpret instructions that don't exist in the i386 baseline architecture?

It should not have any effect (if it does, that's a separate fix). In the disassembler, we explicitly enable the latest instruction set, and I can't think of anything else that would be impacted by it.

In D128410#3604083, @mstorsjo wrote:

I'm not all that familiar with exactly what happens in these APIs here that makes this a nested exception and what other non-GUI APIs would produce the same effect. Maybe any API where Windows system code calls back to a user provided callback? Does @stella.stamenova know?

In D128410#3604075, @labath wrote:

In D128410#3604066, @mstorsjo wrote:

For this testcase to work, it needs to be able to open a window - so it can't run entirely in headless mode. But I guess that a bunch of other tests in LLDB also already do that, since running the LLDB tests on Windows pops up a dozen of windows temporarily.

I think those just come from the tests (or more like test runners) which forget to suppress a console window from opening (see LLDB_LAUNCH_INFERIORS_WITHOUT_CONSOLE) -- this is probably the first "gui" application. This isn't necessarily a showstopper, but I am surprised that it is necessary. I'm hardly a windows expert, but I would expect that it should be possible to generate an exception (even nested exception, which to me sounds like the equivalent of getting a signal inside a signal handler) in a "regular" text-mode application. Are you sure that is not possible?

In D128268#3603931, @labath wrote:

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

For this testcase to work, it needs to be able to open a window - so it can't run entirely in headless mode. But I guess that a bunch of other tests in LLDB also already do that, since running the LLDB tests on Windows pops up a dozen of windows temporarily.

In D128268#3603931, @labath wrote:

As a separate path forward, one could also consider to stop returning
two architecture specs from ObjectFilePECOFF::GetModuleSpecifications
for i386 files.

I think that would be much more preferable. I think the current implementation GetModuleSpecifications is based on a misconception about what the multiple results of that function mean (a fat binary -- as you've discovered).

We wouldn't want to return armv6-*-* (or armv7, v8, ...) for a file that claims it can be run on armv5. Elf files also return just a single result for i386 file.

I think this might have broken a bunch of testcases on the lldb-x64-windows-ninja buildbot too, e.g. https://lab.llvm.org/buildbot/#/builders/83/builds/20304 (CC @stella.stamenova)

In D127436#3602224, @stella.stamenova wrote:

This broke the Windows LLDB bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/20295/steps/7/logs/stdio

In D127436#3602224, @stella.stamenova wrote:

This broke the Windows LLDB bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/20295/steps/7/logs/stdio

Thanks, this looks reasonable to me!

Refactored the code to add the both_windows variable as suggested (which turned out much nicer, thanks!).

In D128268#3601104, @DavidSpickett wrote:

LGTM. I like the ifs the way I suggested but up to you, either way it's not the easiest logic to parse.

In D126313#3600017, @beanz wrote:

LGTM.

Use elseif (NOT CMAKE_CROSSCOMPILING), skipping the dummy empty initialization.

In D128294#3599885, @BertalanD wrote:

oops, no

missing a REQUIRES line

Keep passing the compiler to the nested cmake when not cross compiling.

In D126313#3599863, @beanz wrote:

I think this behavior change should only be made if CMAKE_CROSSCOMPILING=True. There are common cases where people configure LLVM targeting the host using a compiler that isn't the default host compiler (think compiling on Linux using clang). Since the Native toolchain is also used for using optimized tablegen when building a debug configuration, this code isn't strictly for cross-compilation.

@phosek - Can you follow up on this one?

Added a comment to the testcase.

In D128268#3598967, @DavidSpickett wrote:

However, after this commit, if the object file is advertised with the different environment (either when built in a mingw environment, or if that setting is set), the fat binary validation won't accept the file any longer.

Is "this commit" referring to https://reviews.llvm.org/rG25c8a061c5739677d2fc0af29a8cc9520207b923 or to the change we're reviewing here?

This breaks compilation for me, causing hangs when compiling some source files. Repro with https://martin.st/temp/rdopt-preproc.c, built with clang -target aarch64-w64-mingw32 -w -c -O3 rdopt-preproc.c. Previously this completed in ~5 seconds, now it doesn't terminate.

In D127436#3574289, @alvinhochun wrote:

Does this test look good:

This LGTM, it seems to build correctly for me in all my test environments!

In D127975#3592242, @mstorsjo wrote:

If the issue is that compiling with -m32 generally seems to work, but fails in practice due to some header missing, perhaps the cmake test should be extended to try compiling a file that includes that header?

If the issue is that compiling with -m32 generally seems to work, but fails in practice due to some header missing, perhaps the cmake test should be extended to try compiling a file that includes that header?

LGTM

Looks reasonable!

This looks reasonable to me!

(Side question: What's the correct -commits list for discussing compiler-rt patches? They don't get autoassigned any, but probably should, for visibility.)

Looks good to me in general, one general question though.

In D126676#3587243, @rnk wrote:

I don't have a great answer here, but yes, dllexport macro norms sort of run directly counter to the normal ways that people use PCH. It seems like projects will need to have a library module / PCH file for building a DLL and then a PCH / module for consuming the DLL.

The changes left in this patch seem fine to me now!

LGTM in general, but I'd leave it to the others to formally approve.

@rnk - What do you think about this? See the later discussion in D126898.

It’s also worth pointing out that being off this list doesn’t mean that code for the platforms is ripped out - there’s many platforms that are supported on a best effort basis.

In D126291#3577133, @rovka wrote:

I had the same idea about switching the tests to using target triples instead of having separate files for it, but initially I had some issues getting that to work properly. When specifying a triple, we need to provide an architecture. Leaving the triple as unkown-linux-gnu or just linux-gnu gives us an error along the lines of flang-new: error: unknown target triple 'unknown-unknown-linux-gnu', please use -triple or -arch. OTOH, hardcoding an architecture like x86 or aarch64 fails if we're not building that specific backend.

In D126291#3573937, @mmuetzel wrote:

Ah ok. So instead of having ! REQUIRES: lines, those tests should add -target x86_64-windows-msvc to the flang invocation?

In D126291#3573742, @mmuetzel wrote:

In D126291#3573607, @mstorsjo wrote:

FWIW, something very similar was added just a couple days ago in an lldb specific lit.cfg.py: https://reviews.llvm.org/D127048#change-KJ7QgKPHtN1S

Thank you for pointing this out.
Those feature definitions for the lldb subproject probably stem from around here:
https://github.com/llvm/llvm-project/blob/main/lldb/test/Shell/lit.cfg.py#L65

Instead of every subproject adding their own conditions for similar features, would it make sense to move these definitions to somewhere where all subprojects could use those same lit features? Would that be in /llvm/utils/lit/lit/llvm/config.py?

In D126291#3573137, @mmuetzel wrote:

Not sure if this is the right place to add this. But maybe something like this could be used to add 'windows-msvc' and 'windows-gnu' features that could be used to run tests conditionally on Windows with MSVC or MinGW toolchains:

diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index b65316128146..8b911997a876 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -134,6 +134,10 @@ class LLVMConfig(object):
                 features.add('target-aarch64')
             elif re.match(r'^arm.*', target_triple):
                 features.add('target-arm')
+            if re.match(r'.*-windows-msvc', target_triple):
+                features.add('windows-msvc')
+            elif re.match(r'.*-windows-gnu', target_triple):
+                features.add('windows-gnu')
 
         use_gmalloc = lit_config.params.get('use_gmalloc', None)
         if lit.util.pythonize_bool(use_gmalloc):`

In D126898#3572344, @glandium wrote:

So, looking a little bit deeper, the resource editor in NSIS doesn't handle relocations, and the .reloc section is right after the .rsrc section that the resource editor will grow... So they actually have a build rule to pass /FIXED to link.exe when building their stubs with MSVC. I suppose mingw-binutils defaults to non-relocatable .exes (because the problem doesn't appear when building with mingw-gcc/mingw-binutils), and we only end up with relocatable .exes because we're building with mingw-clang/lld and lld defaults to relocatable .exes.

In D123408#3571386, @spatel wrote:

In D123408#3571242, @syzaara wrote:

Oh nevermind, please ignore above comment, I see what you are saying now! That we can add a transformation which can convert:

Right - see if the motivating example looks better with:
afa192cfb604

In D126898#3571941, @glandium wrote:

The base relocation table usually resides in the .reloc section, and as the exact size (0xB68) matches, it looks like it just has got the wrong RVA in the header. With the new, more thorough checks, libObject notices that the RVA points into the .ndata section (within 0x40000 - 0x5C000) but there's only raw data covering 0x40000 - 0x40200, and 0x43000 is outside of that. Previously we'd just set up a pointer into bogus/out of range data in these cases, now we error out.

From a cursory look, it seems to come from NSIS itself not handing BaseRelocationTableRVA. Its prebuild binaries come with stubs that actually have it set to 0 and thus don't have a .reloc section. The stubs we're using in the case of that helper.exe were compiled with mingw-clang and linked with mingw-lld, and do contain a .reloc section. The helper.exe binary ends up having the same BaseRelocationTableRVA as the stub, but NSIS moved the .reloc section, so it's not correct anymore.