@abrachet I left some comments/questions. When putting a patch up for review in phabricator please include the message that would be in the commit in the review to help reviewers understand your change and why it's being made.

@abrachet This change looks reasonable but it could really do with an actual description of the change being made and why.

@ahatanak Thanks for working on this. I think that annotating these particular tests so that's clear that they are supposed to crash (and therefore symbolication is of no use) is a good change. If we want to make a more global change for tests I think that should be handled separately.

LGTM

@thetruestblue It looks like the new test in this change fails https://green.lab.llvm.org/green/job/clang-stage1-RA/33833/

In D144688#4165144, @kparzysz wrote:

This broke Lua support:

In file included from /w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp:9:
/w/src/llvm.org/lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h:95:68: error: non-virtual member function marked 'override' hides virtual member function
                                    const char *command_body_text) override;
                                                                   ^
/w/src/llvm.org/lldb/include/lldb/Interpreter/ScriptInterpreter.h:415:16: note: hidden overloaded virtual function 'lldb_private::ScriptInterpreter::SetWatchpointCommandCallback' declared here: different number of parameters (3 vs 2)
  virtual void SetWatchpointCommandCallback(WatchpointOptions *wp_options,
               ^

Thanks for the review and approval. I've the fixed the nits so I'm going to land.

• Fix nits

@JDevlieghere @mib @bulbazord Thanks for all the feedback. This patch is ready for another review pass.

• Add WatchpointValueKind enum
• Use WatchpointValueKind for new GetWatchValueKind() method (previously named SBWatchpoint::IsWatchVariable)
• Add IsWatchingsRead() method
• Add IsWatchingWrites() method
• Remove m_cached_watch_spec to avoid breaking ABI.
• In GetWatchSpec() use ConstString rather than returning a pointer to owned by the now removed m_cached_watched_spec.

In D144937#4159324, @jasonmolenda wrote:

Hi Dan, I hadn't looked this over very closely but one thing that jumped out is that you're adding a member to SBWatchpoint, and we can't do that, it's an API breaking change. In cases where we need to store additional information than a weak pointer to an lldb private object, we traditionally add an Impl class which has the additional member(s) and a shared pointer to the lldb private object that backs the class. e.g. see SBValue's ValueImpl, the definition is in SBValue.cpp.

@mib Thanks for working on this. LGTM (with very minor nits), but you should probably wait for someone who works on LLDB more frequently than me to give you the ok.

Fix typo

Fix reviewers

LGTM. Thanks for addressing my comments.

@thieta Thanks for working on this. I don't think can land as is because I'm not convinced this works correctly. Please see my comments.

LGTM

Overall approach LGTM. I just have some very minor nits.

In D137024#3931488, @mgorny wrote:

Well, I'm certainly not opposed to making all the paths configurable. However, I'm not sure if having CMake file accessible one way or another wouldn't eventually be a necessity. For one thing, I would like to move more common code from standalone build codepaths of individual projects into a dedicated CMake file in LLVM (and while I don't want to speak of others, it seems that there are at least few other people who would like to see something similar done). I suppose this wouldn't be an outright blocker if llvm/cmake/Modules directory were present but I'm not 100% sure.

In D137024#3931167, @mgorny wrote:

In D137024#3931126, @thetruestblue wrote:

This patch removes the logic that sets the binary tools dir using llvm-config. We don't have llvmConfig.cmake in our toolchain or our build tree and relied on llvm-config to
set(LLVM_TOOLS_BINARY_DIR "${LLVM_TOOLS_BINARY_DIR}"directory.

While I sympathize with you, I don't think this is valid reason to maintain full compatibility with llvm-config. Unless i'm mistaken, its use for building other LLVM projects standalone was deprecated for a few years already, and compiler-rt was the last project to carry the compatibility code. Furthermore, skipping cmake files from LLVM seems wrong.

LGTM

Other than minor issue in the test this LGTM

This definitely needs some more eyes from Apple engineers. Unfortunately @vsk isn't too active on this part of the compiler anymore. I don't work on PGO myself or TAPI so I can't approve this change. I've added @arphaman and @cishida who might have opinions here.

@yln @kubamracek @thetruestblue @rsundahl @wrotki Adding you as reviewers as I don't work on the Sanitizers much anymore.

I don't think this is the right fix. Having a broken SDK setup is a real problem that should not be hidden by changing this to a warning.

Really nice refactor. LGTM.

The change is right in "spirit" but I think the current implementation will likely break things (or will work by accident). There is a fundamental difference between how compiler-rt is built for Apple platforms and other platforms. For Apple platforms we build all platforms and targets in a single CMake configure (IIUC other platforms have a CMake configure correspond to a single platform and arch). So there is no single sysroot or target triple.

@rjmccall

Sorry if these are silly questions and if I've misunderstood something, I saw n1863 say "functions return unqualified types" and I was very surprised.

Just to be clear, you understand that this is only about top-level qualifiers on the return type, right? const void *foo(); is still meaningful, it's just that const void * const foo(); isn't.

@aaron.ballman

Sorry if these are silly questions and if I've misunderstood something, I saw n1863 say "functions return unqualified types" and I was very surprised.

These are not at all silly questions, so thank you for asking them!

@aaron.ballman Hey I just saw this change and had questions about it. For others looking I think the resolution to DR423 is in https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1863.pdf, I found https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2148.htm#dr_423 hard to parse.

@yaxunl Thanks for addressing my feedback so quickly. I think the commit message should also mention that KEYCUDA is now included in KEYALL. Other than that LGTM.

@smeenai Thanks for the patch. I think the overall spirit of this change is good. There are just some details to work out which I've left in other comments.

In D124643#3487122, @beanz wrote:

The other issue should be fixed in rGb26e44e623c75c084e865084b18541c6a1736df2.

In D124643#3486838, @nemanjai wrote:

In D124643#3486603, @beanz wrote:

Bots should be fixed with rG55e13a6bc0d6a31afc258a012184253041b0eb8e.

This isn't the only problem. There are also failures due to:

llvm-project/llvm/lib/Object/DXContainer.cpp:42:12: error: call to deleted constructor of 'llvm::Error'
    return Err;
           ^~~

See https://lab.llvm.org/buildbot/#/builders/21/builds/40034/steps/5/logs/stdio

I spoke to a few other engineers in Apple. They are

Making it an ERROR and providing an option to downgrade it to a WARNING seems reasonable to me. Thoughts?

Change LGTM.

I'd definitely prefer moving towards LLVM_ENABLE_RUNTIMES. We already require LLVM_ENABLE_RUNTIMES for libc++, libc++abi and libunwind and I'm going to propose doing the same for compiler-rt as well.

Patch relanded as a680c212cb213bf73be7d3e2ee919fdc743cef0c

Approved.

In D119174#3469454, @bc-lee wrote:

Okay. I understand. Sorry for broke your code.

Reverted in 3469cb14e2316a1e3cf64db5be3738379d9daa8d

@bc-lee This has broken some of Apple's internal builds. You didn't have a single person from Apple approve this code before landing it which is not acceptable given that it affects only Apple platforms. Please CC @yln @thetruestblue @kubamracek @rsundahl and me in the future if you want to land something like this.

@aaron.ballman Thanks.

In D123475#3443283, @kubamracek wrote:

+ more Apple folks

Makes sense, the only thing I can think of is if someone wants to build compiler-rt with older Xcode/SDKs. Not sure if we care about that.

LGTM

Changes seems good. I certainly like that we've simplified things. If we can resolve the question I asked then I think this will be good to go.

Have you considered the different approach of having conditional RUN lines instead? E.g.

@yln I like the approach here as it solves problem for all of LLVM, not just ASan specifically :). This is better than what I was going to do originally. I think the only missing there here are some tests.

Change seems reasonable but I don't have expertise on this code. I've left a few minor nits.