@labath @aleksandr.urakov Does this version look good to you?

+1 the change looks sensible to me - but I'd leave it to someone else of the runtime projects' maintainers (@mclow.lists @ldionne @EricWF) to approve.

Moved the check up to ObjectFilePECOFF.

In D77000#1949247, @aleksandr.urakov wrote:

Hello! But does the format of an exception directory for aarch64 differ completely from x86-64 one?

@rnk Does this form of the change look decent to you?

In D76951#1946998, @amccarth wrote:

Negative values, especially -1, for unneeded IDs is indeed a very things in Windows resource files, not just for menu items (and submenus) but also for decorative controls on dialogs. So this looks fine to me, though we might eventually have to relax the constraints even more.

Looks good I think. I'm not very familiar with the GC parts of LLD, but the explanations sound sensible to me, and I believe this solution has been vetted by at least two others already (bug reporter and @rnk).

Changed hasLinkOnceDirective into avoidWeakIfComdat and adjusted the condition.

In D44543#1942495, @rnk wrote:

In D44543#1922223, @mstorsjo wrote:

@rnk - what do you think about the two alternative solutions I commented above?

The second change seems simpler, but maybe we should adjust the linkonce predicate name. As I understand it, both solutions will preserve the behavior of existing linkonce* and weak* linkages if a comdat is present, which it pretty much always is unless the user is using the __attribute__((weak)) extension.

It seems to me like the hasLinkOnceDirective predicate isn't really helping us here. I dug into the history, and I see that back around 2010, it controlled the emission of .linkonce discard. It got moved to section emission in rG76a07580a. Then, later, when we made all the object file sections use the standard, non-suffixed names, we folded that into the .section directive and stopped using the .linkonce directive entirely. So, it seems like maybe the MCAsmInfo feature that we really want here should read something like "avoidWeakIfComdat" or "shouldWeakComdatSymbolsBeExternal" or "isWeakComdatExternal" or something like that. shouldUpgradeWeakComdatToExternal...

Or would it be best to extend the generic MCAsmInfo with another function, and change the existing condition into (MAI->hasLinkOnceDirective() && (GV->hasComdat() || !MAI->linkOnceDirectiveRequiresComdat()))? That makes the condition both generic and clearer.

Ping @rnk

@rnk?

@thakis?

Rebased the patch, adjusted the condition to another way of expressing roughly the same, added a comment to the common code.

Superseded by D75989.

@rnk - what do you think about the two alternative solutions I commented above?

Fixed a clang-tidy warning.

In D73534#1918818, @djtodoro wrote:

In D73534#1916309, @djtodoro wrote:

In D73534#1916291, @djtodoro wrote:

Thanks for reporting this! Since this is the case of the X86::MOV16ri the D75326 will solve this issue.

The alternative is D75974.

The D75974 is commited.

I keep running into this issue in various places, that __attribute__((weak)) doesn't work on COFF in clang.

This broke compiling for mingw, repro.c:

a(short);
b() { a(1); }

clang -target x86_64-w64-mingw32 -c repro.c -g -O2, which gives Assertion !MI.isMoveImmediate() && "Unexpected MoveImm instruction"' failed.`

@thakis?

Got to test this now as well, and it works fine for windows at least. Thanks!

LGTM (but I didn't have the possibility to test it right now, but it seems right).

This broke building for windows, and I would be pretty sure it also broke things for apple platforms as well (as _LIBUNWIND_SUPPORT_DWARF_UNWIND is defined on both of them, at least on certain architectures).

In D73227#1891931, @epastor wrote:

It's true, MSVC doesn't support inline assembly at all on non-i386 platforms.

However, LLVM currently does support MS-style inline assembly on x86-64, if you use the MS-style asm blocks... it just doesn't quite work as expected. (We've seen a few bug reports to this effect, including http://llvm.org/PR44272 - several of which have said the option of MS-style inline assembly on x86-64 is a big bonus.) Handling that here would move it closer to fully working.

In D73227#1891896, @epastor wrote:

In D73227#1891874, @thakis wrote:

What's the plan for 64-bit inline asm?

It should probably be the same, admittedly. To clarify: isParsingInlineAsm will only be true when parsing inline MS-style assembly? If so, I'll rename it to isParsingInlineMSAsm in this patch, and let the handling trigger off of both.

FWIW, this change broke building for windows. It seemed fairly straightforward to fix though, see D75102.

Ping, could someone with better insight in the CMake setup comment on whether this is the right spot to add a thing like this? I guess this would end up included when building the runtimes (both as part of the monorepo, and when building them standalone, if the llvm cmake files are available), but I think that would be fine for now.

Does this affect the runtimes as well, if built as part of the monorepo build and/or standalone?

Committed and backported. Thanks!

Committed in d6fe253653b7e760f94ca21d0a7ebbfeeee28777 (I forgot the Differential Revision: tag, sorry).

I can try to get it committed soon. What form do you want for the author line, "Cristian Adam <cristian.adam@gmail.com>"?

In D74599#1876507, @amccarth wrote:

In D74599#1875984, @mstorsjo wrote:

you could just as well make it Windows 7 (which is the documented minimum version).

The documented minimum version for CompareStringOrdinal is Vista per https://docs.microsoft.com/en-us/windows/win32/api/stringapiset/nf-stringapiset-comparestringordinal

Are you talking about the documented minimum version of something else?

I don't use the php based arcanist either, I just upload diffs that I make with git diff -U999.

Looks sensible in general, but you could just as well make it Windows 7 (which is the documented minimum version).

LGTM

FYI this caused failed builds for aarch64 (when building with -fno-signed-zeros), hitting unreachable statements in TargetLowering::getNegatedExpression - see https://bugs.llvm.org/show_bug.cgi?id=44892. I guess that's a bug in the aarch64 backend and not with this change in itself, but it was only uncovered by this change...

In D70840#1871932, @labath wrote:

In D70840#1871862, @mstorsjo wrote:

Now someone might argue that the looking up the address of the ldr opcode is wrong, because that is not the actual pc value, and that we should lookup the address of ldr+1. In that case we can point them to the next function (call_noreturn), which ends with a call to a noreturn function. Now if we get a crash in does_not_return(), the address we will get from the backtrace will be one byte after the last opcode of call_noreturn (bl does_not_return()). This means that in the non-windows case, it will not resolve correctly to the call_noreturn even if we apply the usual trick of subtracting 1 from the address to bring it into the range of the calling function.

Hmm... I presume this issue is present now already (and doesn't really change due to normalizing the line tables and ranges on windows, to the same as linux). Not familiar with "the usual trick of subtracting 1 from the address to bring it into the range of the calling function", but wouldn't something like GetOpcodeLoadAddress(pc) - 1 always bring you into the range of the calling function, given a return address?

Lldb uses this when unwinding specifically to deal with the "call as a last instruction" problem, and I'm pretty sure it's not the only tool doing that. This is even described in the DWARF spec (non-normative text):

In most cases the return address is in the same context as the calling address, but that
need not be the case, especially if the producer knows in some way the call never will
return. The context of the ’return address’ might be on a different line, in a different
lexical block, or past the end of the calling subroutine. If a consumer were to assume that
it was in the same context as the calling address, the virtual unwind might fail.

For architectures with constant-length instructions where the return address
immediately follows the call instruction, a simple solution is to subtract the length of an
instruction from the return address to obtain the calling instruction. For architectures
with variable-length instructions (for example, x86), this is not possible. However,
subtracting 1 from the return address, although not guaranteed to provide the exact
calling address, generally will produce an address within the same context as the calling
address, and that usually is sufficient.

Using GetOpcodeLoadAddress(pc) - 1 would work, but this implies that the caller should be passing in "load" addresses, which is in conflict with the premise I have made at the beginning of the paragraph that one should be passing in "code" addresses here. (My attempt at proof by contradiction.)

In D70840#1871745, @labath wrote:

I did not send out any emails for this, but I did talk about this with @dblaikie when I was in the US last week. My take from that is that we should start by "fixing" llvm-symbolizer so that it works reasonably in these cases. I'll use this godbolt link to illustrate what I mean.

If we take a look at the deref function in that link, and try to pass the address of the first instruction ldr r0, [r0] to llvm-symbolizer, then we will get something reasonable in the non-windows case, but we will get nothing in the windows case, because the debug info would claim that the function starts halfway into that opcode. Fixing the tool so that it returns the same thing everywhere sounds reasonable, and since it does both line table and debug_info lookups, this will already answer the main questions we have about where this logic should be implemented. Then we can just copy that in lldb.

In D61314#1871290, @nathanchance wrote:

The Linux kernel hits the assert in markConstant on arm32 defconfig in net/mac80211/sta_info.o, as reported by Linaro's CI:

This causes failed asserts for me. With https://martin.st/temp/substtml-preproc.c, built with clang -target i686-w64-mingw32 -c -O2 substtml-preproc.c, I'm getting this:

Don't use return() on the file level

In D74042#1864714, @thakis wrote:

Maybe only disable this in shared library builds?

@hans - this could be applied on 10.x (it seems to apply cleanly), to fix PR44791.

Simplified the check to if (WIN32)

Committed in https://reviews.llvm.org/rGd5361190993a.

Committed in https://reviews.llvm.org/rG0f99f678febf.

Committed in https://reviews.llvm.org/rG424babb89ad2.

Committed in https://reviews.llvm.org/rG7e518f3159bd.

In D72543#1854571, @hans wrote:

In D72543#1850031, @steven_wu wrote:

@hans I landed this on master. Let me know if I need to do something for release branch. I will wait a bit to see if it breaks any unexpected users.

I've cherry-picked it to 10.x in 674ec1eb16678b8addc02a4b0534ab383d22fa77, figuring it's better to get it in sooner rather than later. Please let me know if you hear about any problems or if there are follow-ups.

Thanks!

In D73839#1854333, @mstorsjo wrote:

In D73839#1854328, @teemperor wrote:

I guess this is causing the windows failure on the bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13320

Yeah, I saw that - highly strange. I'll try to see if I can reproduce similar issues with a similar toolchain, and I'll push a revert soon unless I find some great clue.

In D73839#1854328, @teemperor wrote:

I guess this is causing the windows failure on the bot: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/13320