This looks right to me. (I'm out of office at the moment, but this looks like what I tested in https://github.com/llvm/llvm-project/issues/62719 so it should work.)

In D151188#4367714, @thieta wrote:

I think I asked this at the meeting, but I forgot the answer already: is there a reason the compiler can't pass in the (relative) library paths as linker directives along with the library names in the .obj files?

I am not sure, I thought we only talked about not being possible to pass absolute paths because of reproducibility.

In D151188#4367807, @thieta wrote:

Regarding the target handling, what's your opinion on introducing a new option (for example /target:) with the default value being the host target (that is the same behavior that's used by Clang)?

Not a bad idea!

The target handling, which seems to be the tricky part, is only needed for LLVM_ENABLE_PER_TARGET_RUNTIME_DIR builds, right? And as far as I know that's not used on Windows, so maybe it's not needed?

This broke the tests on Mac, see e.g. https://green.lab.llvm.org/green/job/clang-stage1-RA/34396/consoleFull (or http://crbug.com/1447928)

I also only looked briefly, but it seems like this assumes LLVM_ENABLE_PER_TARGET_RUNTIME_DIR builds? I think that's still off by default on Windows, so maybe it would be enough to start with supporting the non-per-target-runtime-dir case, or we should aim to support both cases.

lgtm

Do you have a suggestion how we can move this patch forward?

IIRC, D150688 + the diff in https://github.com/llvm/llvm-project/issues/62719#issuecomment-1552903385 + upgrading the pre-merge linux bot should take care of all known issues.

Would it make sense to put all these patches in one new review and then test that on Chromium and ask @glandium to test that too. Then we know whether it solves the issues. Do you want me to make a patch or do you want to do it?

In D150107#4356347, @skan wrote:

In D150107#4356140, @hans wrote:

In D150107#4356102, @hans wrote:

skan mentioned this in rG2ef8ae134828: [X86] Remove patterns for ADC/SBB with immediate 8 and optimize during MC…

That caused assert in Chromium builds. See https://bugs.chromium.org/p/chromium/issues/detail?id=1446973#c2 for a reproducer.

I'll revert while that's investigated.

@hans Thanks for the report! If you retested the reproducer before the revert, you would find it already passed after 5586bc539acb26cb94e461438de01a5080513401. So I reapplied the patch.

It failed before b/c the last operand of MCInst ADC/SBBri could be an expression instead of an immediate.

In D144509#4350052, @Mordante wrote:

In D144509#4349921, @thakis wrote:

Reverted this and follow-ups in d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6 for now.

Sorry this is such a pain to land :(

(See also discussion over in D150688)

I'm not happy that the patch needs to be reverted again.

It has taken me a lot of time to contact all buildbots maintainers to get all buildbots updated to the minimal CMake requirement.

In D150107#4356102, @hans wrote:

skan mentioned this in rG2ef8ae134828: [X86] Remove patterns for ADC/SBB with immediate 8 and optimize during MC…

That caused assert in Chromium builds. See https://bugs.chromium.org/p/chromium/issues/detail?id=1446973#c2 for a reproducer.

skan mentioned this in rG2ef8ae134828: [X86] Remove patterns for ADC/SBB with immediate 8 and optimize during MC…

Nice! lgtm

In D144509#4349399, @mstorsjo wrote:

In D144509#4349051, @hans wrote:

In D144509#4347562, @glandium wrote:

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

It is indeed unfortunate that this seems to come with such a fundamental behavior change. (we already have https://github.com/llvm/llvm-project/issues/62719) Do you have a repro for the issue you're seeing?

Since the breakages are piling up, perhaps it's time to revert this.

Unfortunately, much of this is hard to sort out without actually trying to patch forward on main, since many of these issues don't show up easily in e.g. premerge testing or similar (the patch has been tried many times for months before in that form), so I'd appreciate if we'd give it a little more time to actually try to sort out the issues...

In D150688#4349699, @mstorsjo wrote:

In D150688#4349390, @mstorsjo wrote:

In D150688#4349022, @hans wrote:

compiler-rt has seems to have its own thing in https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/compiler-rt/CMakeLists.txt#L398

Oh, I see. This seems to look for the literal form /MD, but after the CMake update, CMake seems to set the option in the form -MD, so if this pattern is updated to check for both / and -, this might start working as expected.

Although, the new policy does place the flag in a different separate cmake variable, so it’s quite possible that it isn’t enough. In any case, with the new policy, the right thing to do is to set the variable (or target property) for deciding what CRT to use, not to manually exchange flags in the variables.

In D144509#4347562, @glandium wrote:

FYI, 65429b9af6a2c99d340ab2dcddd41dab201f399c is causing problems on Windows compiler-rt for some reason I haven't identified yet (with cmake 3.25.1). Which suggests for a same version of cmake, this is actually altering its behavior, which I wouldn't have expected...

In D150688#4347610, @mstorsjo wrote:

Out of curiosity - what are the symptoms that this change fixes, and which build configurations does it affect? (I would expect that regular builds with just llvm+clang would pass at least?) But I do agree that the new behaviour of CMP0091 probably does conflict with ChooseMSVCCRT.cmake.

I expect with CMake 3.15 the selection could be done cleaner, but that can be done in a separate patch by the compiler-rt maintainers.

This broke some asan tests on mac, see https://bugs.chromium.org/p/chromium/issues/detail?id=1445676

In D150117#4326737, @thakis wrote:

JDevlieghere, do you remember why you went with that other approach?

lgtm

lgtm

Nice!

lgtm

lgtm

The AOK_SizeDirective part from 5b37c181291210bedfbb7a6af5d51229f3652ef0
(2014-08) seems unneeded nowadays (the root cause has likely been fixed
elsewhere).

I don't know anything about openmp's build, but I'm very happy if this fixes it, so +1 from me :)

Seems reasonable to me.

In D149610#4312974, @aganea wrote:

@hans @alvinhochun The rationale is to simply remove noise from the output when there are sparse ordinals in the table.

Can you share what future change this will simplify? The current output seems natural in a way.

This broke the mac tests again, see e.g. the end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8782525813244192609/+/u/package_clang/stdout

lgtm

Thanks! I like it.

We're seeing build failures in Chromium: https://crbug.com/1440126
It's not clear why it's only in some builds, but the linker complains about duplicate definitions of placeholders::__ph<1>: both from libc++'s bind.cpp and from some Chromium file which presumably includes __functional/bind.h.

In D115907#4295923, @paulkirth wrote:

2. Due to inlining etc., it often gets the source locations wrong, which means it points at code where again there were no expectations -- but perhaps that code got inlined into an expectations somewhere else. (e.g. https://crbug.com/1434989#c9)

The checking depends somewhat on the instrumentation type (frontend vs. backend instrumentation) In the case of frontend instrumentation, when the expect intrinsic is lowered (very early in the pipeline) we can report the diagnostic right away, since branch weights from profiling have already been attached. The should mean that you should get fairly accurate source information, since this happens before any optimizations run.

We gave this a try in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=1434989

Heads up that this broke linking of Chromium: http://crbug.com/1435480 We don't have any analysis of what's happening yet, though.

+aeubanks fyi

lgtm

This broke the lit tests on Mac: LeakSanitizer-AddressSanitizer-x86_64 :: TestCases/Darwin/trampoline.mm
@lgrey who added that in D129385 maybe has some idea of what's going on?

It's still broken after the follow-up, e.g. https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8783975392120859201/+/u/package_clang/stdout

In D147256#4262002, @zequanwu wrote:

In D147256#4261949, @hans wrote:

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.

Could we do the remove_dots on the Clang side, where we can decide based on the LangOpts?

Yes, we can do that. Is there a reason to favor there over here? At here, the object path in llc output can also benefits from remove_dots, not just clang.

This broke the tsan tests on Darwin, see e.g. the end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8784015461553130001/+/u/package_clang/stdout

Well, MSVC cl removes redundant dots so we shouldn't remove llvm::sys::path::remove_dots.

This broke the profile tests on Darwin, see e.g. the end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8784035713603613281/+/u/package_clang/stdout

It's actually worse than my repro above showed. Since "real" initialization of the sret arg can get folded in with the "auto" initialization, this patch can also cause an actually initialized return value to become uninitialized: https://bugs.chromium.org/p/chromium/issues/detail?id=1431366#c8

In D137707#4259567, @ayzhao wrote:

I now have a reproducible (but non-reduced) testcase: https://crbug.com/1431366#c5

This looks like a miscompile; the return parameter is not being initialized if we don't take the branch.

Again not an expert here, but lgtm.

In D147256#4249527, @zequanwu wrote:

• Add a -use-target-path-separator flag for llc.
• Add test for llc with that flag.

Finally, here's a stand-alone reproducer for desktop: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624#c11

In D143637#4237166, @hans wrote:

Heads up that we bisected test failures in Chromium (targeting iOS/x86_64 simulator) to this revision: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624 Hopefully we'll be able to figure out what's going on next week, but if anyone else has seen problems too it would be interesting to know.

In D147073#4240793, @zequanwu wrote:

In D147073#4240017, @hans wrote:

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

Yes.

For 0 ? T<C>{} : T<C>{};, the both branches have valid start location but invalid end location. See comments at https://github.com/llvm/llvm-project/issues/45481#issuecomment-1487267814.

For the std::strong_ordering, I found that DeclRefExpr in the ConditionalOperator's true branch has invalid start and end locations. It might because it's inside a PseudoObjectExpr. Maybe we should simply just skip visiting PseudoObjectExpr, I see that its begin and end location are the same. I'm not familiar with that expression, so, I just handled it by adding checks for validating begin and end locations.

I'm not familiar with this code. I suppose the question is whether it's reasonable for this code to expect that the source locations are always valid or not?

In D145628#4238358, @AdvenamTacet wrote:

This update:

• adds _LIBCPP_HAS_ASAN_CONTAINER_ANNOTATIONS_FOR_ALL_ALLOCATORS FTM,
• updates the description,
• updates the example.

As discussed on Discord, #libcxx.

@hans I hope this also answers your concerns.

I still think we need to address how code which builds against different c++ standard libraries are going to realistically use this. As written, the instructions only work with libc++ versions containing this patch -- how is a developer going to detect that?

In D141882#4233721, @ldionne wrote:

I'll take this for a spin internally to see how much breakage this causes. Can you do the same with Chromium to try and quantify how much pain this causes?

In D147256#4237099, @probinson wrote:

I think we cannot be 100% sure about source paths in a cross-compile situation. Cross-compiling on platform A targeting platform B does not mean your sources and debugger UI are on platform B. My users keep source and debugger UI on platform A, debugging target B remotely. We need to preserve the host pathnames. It is not clear to me that this patch does so.

Heads up that we bisected test failures in Chromium (targeting iOS/x86_64 simulator) to this revision: https://bugs.chromium.org/p/chromium/issues/detail?id=1428624 Hopefully we'll be able to figure out what's going on next week, but if anyone else has seen problems too it would be interesting to know.

Thanks for working on this!

lgtm