lgtm

lgtm

Thanks @coby for finding the simplification!

lgtm

In D35558#815451, @joerg wrote:

They are not allocated RWX, they are allocated RW with the option for later X. I.e. the kernel enforces W^X, but you can request
additional protections for later use. Without that, mprotect with X would be rejected later. There are patching for Linux for similar effect, but I don't think
they ever got merged. libffi is a victim of that on Linux.

Yeah, I spoke too soon. I didn't realize that AllocateRWX now does different things on different OSs. We should revisit that.

lgtm

Looks good

This doesn't seem like an acceptable workaround, surely this regresses functionality for arrays with more than UINT_MAX elements.

• rebase, remove for loop

In D35495#812153, @ruiu wrote:

How about UUID, which seems to be used as a synonym to GUID? (https://en.wikipedia.org/wiki/Universally_unique_identifier)

• Rename WindowsGuid to GUID

• remove CheckGuids hack

• rebase on type server removal

In D35394#810049, @ruiu wrote:

It seems a reasonable simplification, but do you have a change to visit types without it?

Thanks, looks great!

• comments

lgtm

In D35396#809645, @kubamracek wrote:

I'm not suggesting to mkdir a new directory for each test. %T will point to a non-existent directory, just like %t points to a non-existent file. If the tests wants to use %T, it will have to mkdir it.

In D35396#809086, @george.karpenkov wrote:

@rnk Actually it would be quite easy to do the mkdir iff %T is present in the file, if performance is a concern.
I would argue that test runs should be isolated by the test runner, and test authors should not be trying to judge whether it's safe to share a directory.

In any case, inconsistent documentation should be updated one way or the other.

In D35396#809062, @george.karpenkov wrote:

@rnk There are two documentation files. The lit.rst says it's unique, TestingGuide.rst says it's not.
So at least that should be fixed.
In any case, I disagree with your comment: what if the user still wants to have %t to signify a temporary file? Why an extra mkdir?

Tests that have this bug simply did not read the documentation for %T: it is the parent directory of %t, which is a non-existent path unique for the test. Any test that fails this way should do RUN: mkdir %t and use %t instead of %T.

• fix lifetime issues
• add commit method

lgtm

I commented on the clang side that I think it's worth having a single ms_abi calling convention, but I don't think it matters too much for LLVM. The only code that cares about the convention is going to be in lib/Target/{X86|AArch64} anyway. It might be worth a try, though.

• Mark SectionChunk final
• Hoist loop-invariant getSize call

In D35371#808377, @ruiu wrote:

This adds one virtual function call for each relocation. I believe it's negligible, but can you run the linker with and without this patch to see if this is fine?

lgtm

Some non-windows targets using MS extensions define _WIN32 for compatibility with Windows but do not have MSVC compatibility.

lgtm

I read through this, but it's hard to see exactly the simplifications that this will yield. I like the follow-up patch to merge PCRel handling between ELF, COFF, and wasm, though.

lgtm

Hopefully rL307827 addresses the memory allocation consideration.

In D34753#806164, @kkretzsch wrote:

In D34753#805362, @rnk wrote:

I made some minor modifications. Taking the revision to upload them so you can see them.

Thanks for commiting my change. One comment on your changes. The reason why I changed the fallback behavior to calling an assertion instead of the report_fatal_error function was that 'report_fatal_error' itself does some allocation, e.g. constucts a MutexGuard. This is problematic in an OOM situation.

Eventually when we standardize on the lit test runner, this will not be an issue. LLVM can choose what shell syntax it wants to use without worrying about the system shell.

This seems like the right approach to me. It's like the Win x64 "shadow slots" are allocated on the callee side instead of the caller side. We might want to consider implementing /homeparams at some point across the Windows-supporting backends, which will require more FrameLowering surgery.

In D35259#805284, @erichkeane wrote:

Oren discovered this miss to the original implementation. I'd reviewed this internally quite a bit.

The reason for the Win32ABI test is that MSVC 'long double' is actually small enough for SSE registers in this case. I DO now (looking again, sorry Elizabeth) wonder if there is a better way to exclude extended-length LongDouble type? Could we us the 'length' of it instead? @rnk : opinion?