Skip to content

Conversation

@dmpots
Copy link
Collaborator

@dmpots dmpots commented Jul 1, 2025

This commit adds some basic unit tests for the AMDGPU plugin.
It modifies the test configuration to expose the HIPCC compiler
to the make file when the AMDGPU plugin is enabled. It also modifies
the dotest framework so that we can compile .hip files using the
Makefile.rules like we do for other tests.

The unit tests compile a simple hip program and validate that we do
create the gpu target succesfully and that we can hit a breakpoint
in the gpu code.

@dmpots dmpots force-pushed the amd-gpu-plugin-tests branch from 2010208 to 7b365f5 Compare July 2, 2025 20:13
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have realized that I need to change this file as well for NVIDIA. I tried to pass the path of my compiler to the python test file when running with lldb-dotest but it didn't work. It seems that the path is only correctly passed when running check-lldb.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have agnostic GPU settings and allow users to specify a GPU compiler path, GPU linker path and anything else that is needed to compile.

@dmpots dmpots force-pushed the amd-gpu-plugin-tests branch from 1f44585 to 74fcf61 Compare July 8, 2025 02:09
@dmpots dmpots marked this pull request as ready for review July 8, 2025 02:10
Copy link
Collaborator

@walter-erquinigo walter-erquinigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Owner

@clayborg clayborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should thing about a few things here: do we want to make a generic framework for GPU building, or have each vendor (AMD, NVidia, etc) to have to add their own Makefile settings. I would vote we make a generic way to specify a GPU compiler and linker and enable GPU tests for one GPU at a time. See inline comments where I talk about this. The idea is if you are testing for AMD would would specify the GPU compiler in dotest_args.py like:

--gpu-type=amd --gpu-cc=/path/to/rocm/hpicc

And for NVidia:

--gpu-type=nvidia --gpu-cc=/path/to/nvidia/compiler/cc

Then the test suite will run one set of GPU tests. And all GPU makefiles would look similar, but each use their own compiler. Any GPU tests that are specific to the GPU type would be run, but none of the others.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If HIP a valid term for all GPU compilers? Or do we need a separate setting for AMD GPU, NVidia and MTIA?

Rename HIP_SOURCES to GPU_SOURCES?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shoulld make this function name more readable. What does Rocm stand for? So should this be something like getGPUCompilerArgs(self) if this is not specific to AMD. If this is specific to AMD this should be getAMDCompilerArgs(self)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ROCm is the AMD platform for GPU computing. Previously I was passing the root rocm dir, but now I am passing the hipcc compiler path directly . Switched the name to getHipccArgs to match the getNvccArgs used in the nvidia plugin.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is HPICC valid for all GPUs, or just AMD?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this AMD specific? If so, please name this better to specify this like amd_gpu_cc_path. If this is a generic compiler for any GPU then gpu_cc_path. Not sure if we ever expect the test suite to run for more than one GPU? If not, then we should do something like gpu_cc_path. Else we might need a dictionary of gpu name or target triple to the compiler path.

Comment on lines +61 to +69
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be more GPU agnostic. We probably don't want to have each GPU maker having to define they own settings like this.

    group.add_argument(
        "--gpu-cc-path",
        metavar="path",
        dest="gpu_cc_path",
        default=None,
        help=textwrap.dedent(
            """Specify the path to a GPU compiler compiler."""
        ),
    )

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with agnostic GPU compiler settings, we might need to enable a GPU type. That way if we have many tests that are AMD specific and some that are NVidia specific, we can enable one GPU to be tested and then the GPU tests would only run if they have this GPU type.

group.add_argument(
        "--gpu-type",
        type="string",
        dest="gpu_type",
        default=None,
        help=textwrap.dedent(
            """Specify the the GPU directory to test. Valid values should match a GPU test directory name in llvm-project/lldb/test/gpu folder."""
        ),
    )

That way anyone can add a directory like:

llvm-project/lldb/test/gpu/amd
llvm-project/lldb/test/gpu/nvidia
llvm-project/lldb/test/gpu/mtia

And then we can test one of these directories. If the --gpu-type option is not set, not GPU tests will be run. If it is set, we would check for a directory in llvm-project/lldb/test/gpu and if it matches this setting, we will run the tests for that GPU in addition.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/HIP_SOURCES/GPU_SOURCES/ ?

Comment on lines +112 to +113
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a GPUCC defined for the compiler and GPULD for the linker? This way any GPU client can work with this build system?

override CC := $(GPUCC)
override CXX := $(GPULD)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a separate HIPFLAGS and should it be named GPU_CFLAGS? If we only allow something to compile as a GPU binary, then we can just use CFLAGS?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/HIP_SOURCES/GPU_SOURCES/?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all of these flags AMD specific? If they are then we need a way to set the default compiler flags for the specific GPU type that we are using, and this would be best done somewhere that checks the GPU type being tested and sets these options as needed for AMD, NVidia, etc

	$(GPUCC) $(GPUFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the nvcc compiler, which is not clang based, we need a custom version of this line, something like

$(NVCC) $(NVCCFLAGS) -Xcompiler "-MT $@ -MD -MP -MF $*.d" -c -o $@ $<

@walter-erquinigo
Copy link
Collaborator

walter-erquinigo commented Jul 10, 2025

@clayborg , I think we shouldn't go the route of making a generic set of flags in the Makefile for all GPUs. A big reason is that there are multiple compilers that can be used for producing binaries for the same GPU. For example, at some point I'll want tests that will compile both triton and cuda source files, and maybe mojo.
This means that it's better if the Makefile is agnostic of GPUs and simply operates on the notion of compilers and source languages. Then, test files become very generic, to the point at which you could have single python test compile both triton and cuda files without any restrictions.

@dmpots
Copy link
Collaborator Author

dmpots commented Jul 10, 2025

We should thing about a few things here: do we want to make a generic framework for GPU building, or have each vendor (AMD, NVidia, etc) to have to add their own Makefile settings. I would vote we make a generic way to specify a GPU compiler and linker and enable GPU tests for one GPU at a time.

@clayborg I agree with @walter-erquinigo here that we should not try to make a generic set of flags that work for all GPUs. What we have added to the Makefile here (and the version Walter added) is support for a new language and the toolchain to compile that language.

In this PR we are enabling tests to be built from hip sources. It is actually possible to target a CPU with hipcc so it is technically not even restricted to running only on a GPU. It can also be targeted to run on Nvidia GPUs so its possible that we could have some Nvidia tests that use hipcc and some that use nvcc.

Just like we have separate variables and settings for C (CC) and C++ (CXX) I think it makes sense to have separate variables for hip (HIPCC) and cuda (NVCC). These are different languages and so having different make rules to support them seems to fit well with the existing framework.

@clayborg clayborg force-pushed the llvm-server-plugins branch from 1f42d8c to b7b4f53 Compare July 11, 2025 20:54
@dmpots dmpots force-pushed the amd-gpu-plugin-tests branch from 74fcf61 to d3eab42 Compare July 11, 2025 21:33
dmpots added 4 commits July 11, 2025 16:32
This commit adds some basic unit tests for the AMDGPU plugin.
It modifies the test configuration to expose the HIPCC compiler
to the make file when the AMDGPU plugin is enabled. It also modifies
the dotest framework so that we can compile .hip files using the
Makefile.rules like we do for other tests.

The unit tests compile a simple hip program and validate that we do
create the gpu target succesfully and that we can hit a breakpoint
in the gpu code.
@dmpots dmpots force-pushed the amd-gpu-plugin-tests branch from d3eab42 to db489f3 Compare July 11, 2025 23:33
@dmpots dmpots merged commit 0031347 into clayborg:llvm-server-plugins Jul 12, 2025
5 checks passed
clayborg pushed a commit that referenced this pull request Aug 10, 2025
* Add tests for amdgpu lldb-server plugin

This commit adds some basic unit tests for the AMDGPU plugin.
It modifies the test configuration to expose the HIPCC compiler
to the make file when the AMDGPU plugin is enabled. It also modifies
the dotest framework so that we can compile .hip files using the
Makefile.rules like we do for other tests.

The unit tests compile a simple hip program and validate that we do
create the gpu target succesfully and that we can hit a breakpoint
in the gpu code.

* Use dynamic line number for breakpoint

* Use LLDB_ENABLE_AMDGPU_PLUGIN configuration variable

* Rename getRocmArgs to getHipccArgs
pveras pushed a commit to pveras/llvm-project that referenced this pull request Oct 10, 2025
A recent change adding a new sanitizer kind (via Sanitizers.def) was
reverted in c74fa20 ("Revert "[Clang][CodeGen] Introduce the
AllocToken SanitizerKind" (llvm#162413)"). The reason was this ASan report,
when running the test cases in
clang/test/Preprocessor/print-header-json.c:

```
==clang==483265==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7d82b97e8b58 at pc 0x562cd432231f bp 0x7fff3fad0850 sp 0x7fff3fad0848
READ of size 16 at 0x7d82b97e8b58 thread T0
    #0 0x562cd432231e in __copy_non_overlapping_range<const unsigned long *, const unsigned long *> zorg-test/libcxx_install_asan_ubsan/include/c++/v1/string:2144:38
    clayborg#1 0x562cd432231e in void std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>::__init_with_size[abi:nn220000]<unsigned long const*, unsigned long const*>(unsigned long const*, unsigned long const*, unsigned long) zorg-test/libcxx_install_asan_ubsan/include/c++/v1/string:2685:18
    clayborg#2 0x562cd41e2797 in __init<const unsigned long *, 0> zorg-test/libcxx_install_asan_ubsan/include/c++/v1/string:2673:3
    clayborg#3 0x562cd41e2797 in basic_string<const unsigned long *, 0> zorg-test/libcxx_install_asan_ubsan/include/c++/v1/string:1174:5
    clayborg#4 0x562cd41e2797 in clang::ASTReader::ReadString(llvm::SmallVectorImpl<unsigned long> const&, unsigned int&) clang/lib/Serialization/ASTReader.cpp:10171:15
    clayborg#5 0x562cd41fd89a in clang::ASTReader::ParseLanguageOptions(llvm::SmallVector<unsigned long, 64u> const&, llvm::StringRef, bool, clang::ASTReaderListener&, bool) clang/lib/Serialization/ASTReader.cpp:6475:28
    clayborg#6 0x562cd41eea53 in clang::ASTReader::ReadOptionsBlock(llvm::BitstreamCursor&, llvm::StringRef, unsigned int, bool, clang::ASTReaderListener&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&) clang/lib/Serialization/ASTReader.cpp:3069:11
    clayborg#7 0x562cd4204ab8 in clang::ASTReader::ReadControlBlock(clang::serialization::ModuleFile&, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, clang::serialization::ModuleFile const*, unsigned int) clang/lib/Serialization/ASTReader.cpp:3249:15
    clayborg#8 0x562cd42097d2 in clang::ASTReader::ReadASTCore(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, clang::serialization::ModuleFile*, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, long, long, clang::ASTFileSignature, unsigned int) clang/lib/Serialization/ASTReader.cpp:5182:15
    clayborg#9 0x562cd421ec77 in clang::ASTReader::ReadAST(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, unsigned int, clang::serialization::ModuleFile**) clang/lib/Serialization/ASTReader.cpp:4828:11
    clayborg#10 0x562cd3d07b74 in clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, clang::SourceLocation, clang::SourceLocation, bool) clang/lib/Frontend/CompilerInstance.cpp:1805:27
    clayborg#11 0x562cd3d0b2ef in clang::CompilerInstance::loadModule(clang::SourceLocation, llvm::ArrayRef<clang::IdentifierLoc>, clang::Module::NameVisibilityKind, bool) clang/lib/Frontend/CompilerInstance.cpp:1956:31
    clayborg#12 0x562cdb04eb1c in clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, clang::Token&, clang::Token&, clang::SourceLocation, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) clang/lib/Lex/PPDirectives.cpp:2423:49
    clayborg#13 0x562cdb042222 in clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, clang::Token&, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) clang/lib/Lex/PPDirectives.cpp:2101:17
    clayborg#14 0x562cdb043366 in clang::Preprocessor::HandleDirective(clang::Token&) clang/lib/Lex/PPDirectives.cpp:1338:14
    clayborg#15 0x562cdafa84bc in clang::Lexer::LexTokenInternal(clang::Token&, bool) clang/lib/Lex/Lexer.cpp:4512:7
    clayborg#16 0x562cdaf9f20b in clang::Lexer::Lex(clang::Token&) clang/lib/Lex/Lexer.cpp:3729:24
    clayborg#17 0x562cdb0d4ffa in clang::Preprocessor::Lex(clang::Token&) clang/lib/Lex/Preprocessor.cpp:896:11
    clayborg#18 0x562cd77da950 in clang::ParseAST(clang::Sema&, bool, bool) clang/lib/Parse/ParseAST.cpp:163:7
    [...]

0x7d82b97e8b58 is located 0 bytes after 3288-byte region [0x7d82b97e7e80,0x7d82b97e8b58)
allocated by thread T0 here:
    #0 0x562cca76f604 in malloc zorg-test/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:67:3
    clayborg#1 0x562cd1cce452 in safe_malloc llvm/include/llvm/Support/MemAlloc.h:26:18
    clayborg#2 0x562cd1cce452 in llvm::SmallVectorBase<unsigned int>::grow_pod(void*, unsigned long, unsigned long) llvm/lib/Support/SmallVector.cpp:151:15
    clayborg#3 0x562cdbe1768b in grow_pod llvm/include/llvm/ADT/SmallVector.h:139:11
    clayborg#4 0x562cdbe1768b in grow llvm/include/llvm/ADT/SmallVector.h:525:41
    clayborg#5 0x562cdbe1768b in reserve llvm/include/llvm/ADT/SmallVector.h:665:13
    clayborg#6 0x562cdbe1768b in llvm::BitstreamCursor::readRecord(unsigned int, llvm::SmallVectorImpl<unsigned long>&, llvm::StringRef*) llvm/lib/Bitstream/Reader/BitstreamReader.cpp:230:10
    clayborg#7 0x562cd41ee8ab in clang::ASTReader::ReadOptionsBlock(llvm::BitstreamCursor&, llvm::StringRef, unsigned int, bool, clang::ASTReaderListener&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&) clang/lib/Serialization/ASTReader.cpp:3060:49
    clayborg#8 0x562cd4204ab8 in clang::ASTReader::ReadControlBlock(clang::serialization::ModuleFile&, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, clang::serialization::ModuleFile const*, unsigned int) clang/lib/Serialization/ASTReader.cpp:3249:15
    clayborg#9 0x562cd42097d2 in clang::ASTReader::ReadASTCore(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, clang::serialization::ModuleFile*, llvm::SmallVectorImpl<clang::ASTReader::ImportedModule>&, long, long, clang::ASTFileSignature, unsigned int) clang/lib/Serialization/ASTReader.cpp:5182:15
    clayborg#10 0x562cd421ec77 in clang::ASTReader::ReadAST(llvm::StringRef, clang::serialization::ModuleKind, clang::SourceLocation, unsigned int, clang::serialization::ModuleFile**) clang/lib/Serialization/ASTReader.cpp:4828:11
    clayborg#11 0x562cd3d07b74 in clang::CompilerInstance::findOrCompileModuleAndReadAST(llvm::StringRef, clang::SourceLocation, clang::SourceLocation, bool) clang/lib/Frontend/CompilerInstance.cpp:1805:27
    clayborg#12 0x562cd3d0b2ef in clang::CompilerInstance::loadModule(clang::SourceLocation, llvm::ArrayRef<clang::IdentifierLoc>, clang::Module::NameVisibilityKind, bool) clang/lib/Frontend/CompilerInstance.cpp:1956:31
    clayborg#13 0x562cdb04eb1c in clang::Preprocessor::HandleHeaderIncludeOrImport(clang::SourceLocation, clang::Token&, clang::Token&, clang::SourceLocation, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) clang/lib/Lex/PPDirectives.cpp:2423:49
    clayborg#14 0x562cdb042222 in clang::Preprocessor::HandleIncludeDirective(clang::SourceLocation, clang::Token&, clang::detail::SearchDirIteratorImpl<true>, clang::FileEntry const*) clang/lib/Lex/PPDirectives.cpp:2101:17
    clayborg#15 0x562cdb043366 in clang::Preprocessor::HandleDirective(clang::Token&) clang/lib/Lex/PPDirectives.cpp:1338:14
    clayborg#16 0x562cdafa84bc in clang::Lexer::LexTokenInternal(clang::Token&, bool) clang/lib/Lex/Lexer.cpp:4512:7
    clayborg#17 0x562cdaf9f20b in clang::Lexer::Lex(clang::Token&) clang/lib/Lex/Lexer.cpp:3729:24
    clayborg#18 0x562cdb0d4ffa in clang::Preprocessor::Lex(clang::Token&) clang/lib/Lex/Preprocessor.cpp:896:11
    clayborg#19 0x562cd77da950 in clang::ParseAST(clang::Sema&, bool, bool) clang/lib/Parse/ParseAST.cpp:163:7
    [...]

SUMMARY: AddressSanitizer: heap-buffer-overflow clang/lib/Serialization/ASTReader.cpp:10171:15 in clang::ASTReader::ReadString(llvm::SmallVectorImpl<unsigned long> const&, unsigned int&)
```

The reason is this particular RUN line:
```
// RUN: env CC_PRINT_HEADERS_FORMAT=json CC_PRINT_HEADERS_FILTERING=direct-per-file CC_PRINT_HEADERS_FILE=%t.txt %clang -fsyntax-only -I %S/Inputs/print-header-json -isystem %S/Inputs/print-header-json/system -fmodules -fimplicit-module-maps -fmodules-cache-path=%t %s -o /dev/null
```

which was added in 8df194f ("[Clang] Support includes translated to
module imports in -header-include-filtering=direct-per-file (llvm#156756)").

The problem is caused by an incremental build reusing stale cached
module files (.pcm) that are no longer binary-compatible with the
updated compiler. Adding a new sanitizer option altered the implicit
binary layout of the serialized LangOptions data structure. The build +
test system is oblivious to such changes. When the new compiler
attempted to read the old module file (from the previous test
invocation), it misinterpreted the data due to the layout mismatch,
resulting in a heap-buffer-overflow. Unfortunately Clang's PCM format
does not encode nor detect version mismatches here; a more graceful
failure mode would be preferable.

For now, fix the test to be more robust with incremental build + test.
clayborg pushed a commit that referenced this pull request Oct 24, 2025
**Mitigation for:** google/sanitizers#749

**Disclosure:** I'm not an ASan compiler expert yet (I'm trying to
learn!), I primarily work in the runtime. Some of this PR was developed
with the help of AI tools (primarily as a "fuzzy `grep` engine"), but
I've manually refined and tested the output, and can speak for every
line. In general, I used it only to orient myself and for
"rubberducking".

**Context:**

The msvc ASan team (👋 ) has received an internal request to improve
clang's exception handling under ASan for Windows. Namely, we're
interested in **mitigating** this bug:
google/sanitizers#749

To summarize, today, clang + ASan produces a false-positive error for
this program:

```C++
#include <cstdio>
#include <exception>
int main()
{
	try	{
		throw std::exception("test");
	}catch (const std::exception& ex){
		puts(ex.what());
	}
	return 0;
}
```

The error reads as such:


```
C:\Users\dajusto\source\repros\upstream>type main.cpp
#include <cstdio>
#include <exception>
int main()
{
        try     {
                throw std::exception("test");
        }catch (const std::exception& ex){
                puts(ex.what());
        }
        return 0;
}
C:\Users\dajusto\source\repros\upstream>"C:\Users\dajusto\source\repos\llvm-project\build.runtimes\bin\clang.exe" -fsanitize=address -g -O0 main.cpp

C:\Users\dajusto\source\repros\upstream>a.exe
=================================================================
==19112==ERROR: AddressSanitizer: access-violation on unknown address 0x000000000000 (pc 0x7ff72c7c11d9 bp 0x0080000ff960 sp 0x0080000fcf50 T0)
==19112==The signal is caused by a READ memory access.
==19112==Hint: address points to the zero page.
    #0 0x7ff72c7c11d8 in main C:\Users\dajusto\source\repros\upstream\main.cpp:8
    #1 0x7ff72c7d479f in _CallSettingFrame C:\repos\msvc\src\vctools\crt\vcruntime\src\eh\amd64\handlers.asm:49
    #2 0x7ff72c7c8944 in __FrameHandler3::CxxCallCatchBlock(struct _EXCEPTION_RECORD *) C:\repos\msvc\src\vctools\crt\vcruntime\src\eh\frame.cpp:1567
    #3 0x7ffb4a90e3e5  (C:\WINDOWS\SYSTEM32\ntdll.dll+0x18012e3e5)
    #4 0x7ff72c7c1128 in main C:\Users\dajusto\source\repros\upstream\main.cpp:6
    #5 0x7ff72c7c33db in invoke_main C:\repos\msvc\src\vctools\crt\vcstartup\src\startup\exe_common.inl:78
    #6 0x7ff72c7c33db in __scrt_common_main_seh C:\repos\msvc\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #7 0x7ffb49b05c06  (C:\WINDOWS\System32\KERNEL32.DLL+0x180035c06)
    #8 0x7ffb4a8455ef  (C:\WINDOWS\SYSTEM32\ntdll.dll+0x1800655ef)

==19112==Register values:
rax = 0  rbx = 80000ff8e0  rcx = 27d76d00000  rdx = 80000ff8e0
rdi = 80000fdd50  rsi = 80000ff6a0  rbp = 80000ff960  rsp = 80000fcf50
r8  = 100  r9  = 19930520  r10 = 8000503a90  r11 = 80000fd540
r12 = 80000fd020  r13 = 0  r14 = 80000fdeb8  r15 = 0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: access-violation C:\Users\dajusto\source\repros\upstream\main.cpp:8 in main
==19112==ABORTING
```

The root of the issue _appears to be_ that ASan's instrumentation is
incompatible with Window's assumptions for instantiating `catch`-block's
parameters (`ex` in the snippet above).

The nitty gritty details are lost on me, but I understand that to make
this work without loss of ASan coverage, a "serious" refactoring is
needed. In the meantime, users risk false positive errors when pairing
ASan + catch-block parameters on Windows.

**To mitigate this** I think we should avoid instrumenting catch-block
parameters on Windows. It appears to me this is as "simple" as marking
catch block parameters as "uninteresting" in
`AddressSanitizer::isInterestingAlloca`. My manual tests seem to confirm
this.

I believe this is strictly better than today's status quo, where the
runtime generates false positives. Although we're now explicitly
choosing to instrument less, the benefit is that now more programs can
run with ASan without _funky_ macros that disable ASan on exception
blocks.

**This PR:** implements the mitigation above, and creates a simple new
test for it.

_Thanks!_

---------

Co-authored-by: Antonio Frighetto <me@antoniofrighetto.com>
clayborg pushed a commit that referenced this pull request Nov 14, 2025
…am (llvm#167724)

This got exposed by `09262656f32ab3f2e1d82e5342ba37eecac52522`.

The underlying stream of `m_os` is referenced by the `TextDiagnostic`
member of `TextDiagnosticPrinter`. It got turned into a
`llvm::formatted_raw_ostream` in the commit above. When
`~TextDiagnosticPrinter` (and thus `~TextDiagnostic`) is invoked, we now
call `~formatted_raw_ostream`, which tries to access the underlying
stream. But `m_os` was already deleted because it is earlier in the
order of destruction in `TextDiagnosticPrinter`. Move the `m_os` member
before the `TextDiagnosticPrinter` to avoid a use-after-free.

Drive-by:
* Also move the `m_output` member which the `m_os` holds a reference to.
The fact it's a reference indicates the expectation is most likely that
the string outlives the stream.

The ASAN macOS bot is currently failing with this:
```
08:15:39  =================================================================
08:15:39  ==61103==ERROR: AddressSanitizer: heap-use-after-free on address 0x60600012cf40 at pc 0x00012140d304 bp 0x00016eecc850 sp 0x00016eecc848
08:15:39  READ of size 8 at 0x60600012cf40 thread T0
08:15:39      #0 0x00012140d300 in llvm::formatted_raw_ostream::releaseStream() FormattedStream.h:205
08:15:39      #1 0x00012140d3a4 in llvm::formatted_raw_ostream::~formatted_raw_ostream() FormattedStream.h:145
08:15:39      #2 0x00012604abf8 in clang::TextDiagnostic::~TextDiagnostic() TextDiagnostic.cpp:721
08:15:39      #3 0x00012605dc80 in clang::TextDiagnosticPrinter::~TextDiagnosticPrinter() TextDiagnosticPrinter.cpp:30
08:15:39      #4 0x00012605dd5c in clang::TextDiagnosticPrinter::~TextDiagnosticPrinter() TextDiagnosticPrinter.cpp:27
08:15:39      #5 0x0001231fb210 in (anonymous namespace)::StoringDiagnosticConsumer::~StoringDiagnosticConsumer() ClangModulesDeclVendor.cpp:47
08:15:39      #6 0x0001231fb3bc in (anonymous namespace)::StoringDiagnosticConsumer::~StoringDiagnosticConsumer() ClangModulesDeclVendor.cpp:47
08:15:39      #7 0x000129aa9d70 in clang::DiagnosticsEngine::~DiagnosticsEngine() Diagnostic.cpp:91
08:15:39      #8 0x0001230436b8 in llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const IntrusiveRefCntPtr.h:103
08:15:39      #9 0x0001231fe6c8 in (anonymous namespace)::ClangModulesDeclVendorImpl::~ClangModulesDeclVendorImpl() ClangModulesDeclVendor.cpp:93
08:15:39      #10 0x0001231fe858 in (anonymous namespace)::ClangModulesDeclVendorImpl::~ClangModulesDeclVendorImpl() ClangModulesDeclVendor.cpp:93
...
08:15:39
08:15:39  0x60600012cf40 is located 32 bytes inside of 56-byte region [0x60600012cf20,0x60600012cf58)
08:15:39  freed by thread T0 here:
08:15:39      #0 0x0001018abb88 in _ZdlPv+0x74 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x4bb88)
08:15:39      #1 0x0001231fb1c0 in (anonymous namespace)::StoringDiagnosticConsumer::~StoringDiagnosticConsumer() ClangModulesDeclVendor.cpp:47
08:15:39      #2 0x0001231fb3bc in (anonymous namespace)::StoringDiagnosticConsumer::~StoringDiagnosticConsumer() ClangModulesDeclVendor.cpp:47
08:15:39      #3 0x000129aa9d70 in clang::DiagnosticsEngine::~DiagnosticsEngine() Diagnostic.cpp:91
08:15:39      #4 0x0001230436b8 in llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const IntrusiveRefCntPtr.h:103
08:15:39      #5 0x0001231fe6c8 in (anonymous namespace)::ClangModulesDeclVendorImpl::~ClangModulesDeclVendorImpl() ClangModulesDeclVendor.cpp:93
08:15:39      #6 0x0001231fe858 in (anonymous namespace)::ClangModulesDeclVendorImpl::~ClangModulesDeclVendorImpl() ClangModulesDeclVendor.cpp:93
...
08:15:39
08:15:39  previously allocated by thread T0 here:
08:15:39      #0 0x0001018ab760 in _Znwm+0x74 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x4b760)
08:15:39      #1 0x0001231f8dec in lldb_private::ClangModulesDeclVendor::Create(lldb_private::Target&) ClangModulesDeclVendor.cpp:732
08:15:39      #2 0x00012320af58 in lldb_private::ClangPersistentVariables::GetClangModulesDeclVendor() ClangPersistentVariables.cpp:124
08:15:39      #3 0x0001232111f0 in lldb_private::ClangUserExpression::PrepareForParsing(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, bool) ClangUserExpression.cpp:536
08:15:39      #4 0x000123213790 in lldb_private::ClangUserExpression::Parse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) ClangUserExpression.cpp:647
08:15:39      #5 0x00012032b258 in lldb_private::UserExpression::Evaluate(lldb_private::ExecutionContext&, lldb_private::EvaluateExpressionOptions const&, llvm::StringRef, llvm::StringRef, std::__1::shared_ptr<lldb_private::ValueObject>&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, lldb_private::ValueObject*) UserExpression.cpp:280
08:15:39      #6 0x000120724010 in lldb_private::Target::EvaluateExpression(llvm::StringRef, lldb_private::ExecutionContextScope*, std::__1::shared_ptr<lldb_private::ValueObject>&, lldb_private::EvaluateExpressionOptions const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>*, lldb_private::ValueObject*) Target.cpp:2905
08:15:39      #7 0x00011fc7bde0 in lldb::SBTarget::EvaluateExpression(char const*, lldb::SBExpressionOptions const&) SBTarget.cpp:2305
08:15:39  ==61103==ABORTING
...
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants