I'll go ahead and land this one tomorrow, before the branch; it's harmless for other configurations and makes the code more consistent. As the locale support in that crt (msvcrt.dll) is so lacking, I don't see a good way to make a meaningful testcase - the main takeaway of the change is consistency.

@rnk and/or @rsmith - can either of you have a look at the new version of this?

LGTM, but I think we should do the same for RangeExtensionThunkARM as well (with alignment 2).

In D69554#1731282, @mclow.lists wrote:

Shouldn't there be a test for this new behavior?

FWIW, I looked into tests, and this seems to fix at least 11 failing tests in libc++ on windows.

@rsmith - does this look right to you?

Making the tls wrappers comdat on any platform that supports comdat; updated tests accordingly.

Ping @rsmith

In D71037#1803574, @Tyker wrote:

In D71037#1803361, @xbolva00 wrote:

Can you add a test case for that crash? Otherwise OK.

as i said. the bug can only occur for one line files. so we can't have a run line and a the crashing line. so i wasn't able to make a test for it.

In D71711#1802223, @rnk wrote:

I guess if the absolute symbol change is enough to unblock you, I'd leave this alone for now. The gnu behavior seems error prone and difficult to implement.

Builds fine in my MinGW configuration now, thanks!

In D71037#1799792, @mstorsjo wrote:

This broke my builds.

This broke my builds. A configure test that tries to compile int main (void) { for( int i = 0; i < 9; i++ ); return 0; } (a single line source file), with -Wall enabled, now triggers failed asserts:

In D71711#1798098, @mstorsjo wrote:

Are there any other options then, than trying to do what GCC/binutils does, by changing the suffix .default into something that more hopefully is unique? They use the name of another defined (probably non-comdat) external symbol from the same object (that should be unique) as suffix. But that feels like a much larger mess to implement.

FWIW, right now this is blocking me from llvm with clang itself for mingw targets, since we started using an external thread_local variable.

Ok, so apparently this doesn't work, as MS link.exe rejects object files, where a IMAGE_SYM_CLASS_WEAK_EXTERNAL symbol points at a symbol which isn't external - so I had to revert it.

Did as suggested. As some of the moved blocks are larger than what was left unmoved, parts of the diff is a bit nonobvious though.

Ping @rnk

In D70840#1792498, @labath wrote:

In D70840#1791292, @mstorsjo wrote:

And irrespectively if the ArchSpec vs Architecture design, can you (either of you) comment on the updated form of the patch?

The code still seems somewhat schizophrenic to me. :/ The line tables are fixed up super late,

In D71572#1788786, @rnk wrote:

Looks like @rsmith did this here:
https://github.com/llvm/llvm-project/commit/fbe2369f1a514423e4c25417ab3532502fde6f2a

I see that it was replacing a CHECK for a specific comdat group with no comdat at all. I *think* that's not correct, we should be checking for the trivial comdat group, the one for the wrapper function, not the comdat group of the TLS variable. Let's get input from Richard, though.

@clayborg - can you follow up on @labath's reply here?

Ok, so if you're in agreement on this one, can either of you give the stamp of approval? :-)

In D71650#1789897, @rjmccall wrote:

Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM.

Changed to make it a comdat; unless I'm mistaken, neither linkonce_odr nor weak_odr actually make it possible to link multiple copies of the symbol on windows, unless it's made a comdat. With that in place, the previous change (weak_odr to linkonce_odr) actually isn't needed.

Actually, this doesn't seem to be enough for the case at hand (used with -ffunction-sections -fdata-sections), I'll have to continue looking into it tomorrow.

Ping

Admittedly, this python doesn't really pass all tests anyway, as some tests rely on path formatting to match between different components - but it does help running some number of tests with it at least.

Added a code comment, using llvm::path::has_extension, added testcases.

In D70840#1782951, @clayborg wrote:

So my idea would be:
1 - add all virtual functions to from lldb_private::Architecture as normal members of ArchSpec.
2 - have ArchSpec grap the the lldb_private::Architecture using info in the ArchSpec when it is needed for methods added in step 1 and still call through to the lldb_private::Architecture plug-ins

Added ArchSpec::GetOpcodeLoadAddress() (didn't refactor Architecture to use it yet, so this is still duplicated code with the ArchitectureArm and ArchitectureMips plugins, but posting for feedback).

In D70450#1780211, @fhahn wrote:

Yeah, there were some cases with non-debug $noreg operands. Fixed by rG2675a3c8806a

In D70450#1779903, @fhahn wrote:

In D70450#1779886, @fhahn wrote:

In D70450#1779756, @phosek wrote:

We're seeing an assertion error in Clang when compiling compiler-rt builtins for aarch64-linux-gnu with this change:

FAILED: CMakeFiles/clang_rt.builtins-aarch64.dir/multc3.c.o 
/b/s/w/ir/k/recipe_cleanup/clanga0lNQB/llvm_build_dir/./bin/clang --target=aarch64-unknown-linux-gnu --sysroot=/b/s/w/ir/k/cipd/linux-arm64 -DVISIBILITY_HIDDEN  -O2 -g -DNDEBUG    -std=c11 -fPIC -fno-builtin -fvisibility=hidden -fomit-frame-pointer -MD -MT CMakeFiles/clang_rt.builtins-aarch64.dir/multc3.c.o -MF CMakeFiles/clang_rt.builtins-aarch64.dir/multc3.c.o.d -o CMakeFiles/clang_rt.builtins-aarch64.dir/multc3.c.o   -c /b/s/w/ir/k/llvm-project/compiler-rt/lib/builtins/multc3.c
clang-10: /b/s/w/ir/k/llvm-project/llvm/include/llvm/MC/MCRegisterInfo.h:677: llvm::MCRegUnitIterator::MCRegUnitIterator(llvm::MCRegister, const llvm::MCRegisterInfo *): Assertion `Reg && "Null register has no regunits"' failed.
clang-10: error: unable to execute command: Aborted
clang-10: error: clang frontend command failed due to signal (use -v to see invocation)

Looks like the patch missed to skip debug operands in an assertion. I will push a fix shortly.

Should be fixed by rG4fe92abceb9a

In D70840#1763639, @labath wrote:

• I think we ought to have some kind of a utility function to hold the logic for the &~1 stuff. there is something like that in Architecture::GetOpcodeLoadAddress. The Architecture class is mostly independent of a target, so we may be able create one instance for each module or symbol file, but that feels quite heavy. I might even go for putting something like that into the ArchSpec class. The raison d'être of the Architecture class was to evict some heavy code out of ArchSpec -- this was ArchitectureArm::OverrideStopInfo. That one is definitely too heavy, so still don't thing it belongs in ArchSpec, but the &~1 thingy seems like it could find a place there.)

@labath does this one seem sensible and like what you suggested in D70848?

In D71117#1774395, @broadwaylamb wrote:

@mstorsjo thanks for verifying!

When preparing this patch I've looked through the flags being added and couldn't find those that would benefit from this.

1. Obviously, this won't affect any -W flags.
2. Same for -fstrict-aliasing.
3. -EHsc is MSVC-specific and won't generate any new calls (will it?).
4. -fno-exceptions and -fno-rtti won't add any new calls either.
5. Same for -nostdinc++.
6. -D and -U are only for the preprocessor.
   
   So, it seems like here we only want -funwind-tables to be guarded with this CMake setting.

but it still might be good for consistency in any case?

Now that you've demonstrated the issues that this flag might uncover, my opinion is we probably only want this flag to be set in places we know for sure it is needed, with a clear explanation why it is needed.

Thanks - this one seems to work fine for my use case at least.

In D70840#1767611, @mstorsjo wrote:

In D70840#1767049, @labath wrote:

In D70840#1767028, @mstorsjo wrote:

In D70840#1765373, @clayborg wrote:

So some background on how address masks are handled in LLDB:

Typically the way we have tried to take care of the extra Thumb bit for ARM using:

lldb::addr_t Address::GetCallableLoadAddress(Target *target, bool is_indirect = false) const;
lldb::addr_t GetOpcodeLoadAddress(Target *target, AddressClass addr_class = AddressClass::eInvalid) const;

The first will add the extra bit to an address if needed. The latter will strip the bit if needed. This does require a target though and the target uses the "Architecture" class for ARM to do the work of using the mask. Not sure if we want to try to get an architecture class and use that here for stripping the bit instead of using an address mask?

So, where do I get hold of an Architecture* object instance from within the relevant contexts (within SymbolFileDWARF, where we have a reference to the object file)? Also within source/Symbol/DWARFCallFrameInfo.cpp where we have existing code that does the same.

Well... that's the tricky part (which I've tried to allude to in D70840#1763639. Currently the only thing holding an architecture object is the target class, but since one of the goals of the SymbolFile stuff is to be independent of any particular target, you cannot easily get hold of that. That is why I tried to steer this towards having this in the ArchSpec class. If we don't want that, we'll probably have to create one new Architecture instance local to each Module object...

Ah, sorry for overlooking those parts of your comment. @clayborg - what do you think of @labath's suggestions here?

In D69950#1771515, @rnk wrote:

I fixed this particular code upstream:
https://github.com/KhronosGroup/glslang/pull/2010
I am not enough an expert to be sure, but I suspect this is in the area of "invalid, no diagnostic required", where this code is invalid, but a conforming C++ implementation could either reject or accept it. Now we reject it, and that seems better in the long term, even though it creates a fire drill in the short term. =(

In D69950#1771133, @eandrews wrote:

In D69950#1770138, @mstorsjo wrote:

This (when reapplied in https://reviews.llvm.org/rG878a24ee244a24c39d1c57e9af2) broke compilation of code that earlier built fine. A reduced example:

namespace glslang {
class TPoolAllocator {
  void operator=(TPoolAllocator);
};
template <class> class a {
  TPoolAllocator *b;
  void c() { allocator = *b; }
  TPoolAllocator allocator;
};
} // namespace glslang

With this patch, some errors in templates are diagnosed earlier (i.e. does not wait till instantiation). Since 'allocator' and 'b' aren't dependent, I think this is a valid diagnosis. GCC throws an error on this code upon instantiation. https://godbolt.org/z/X9Y-Vy