Skip to content

[Indexing] Don't verify mangling of USRs #81941

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jun 3, 2025

Verifying USR mangling adds ~30% overhead to indexing times. Since an incorrect USR mangling doesn't result in a correctness issue at the same level as a miscompile, save those 30% in non-assert builds.

@ahoppen ahoppen requested review from bnbarham and hamishknight June 3, 2025 14:44
@ahoppen ahoppen force-pushed the dont-verify-mangle branch from 3d628b3 to ca2060f Compare June 3, 2025 14:46
@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2025

@swift-ci Please smoke test

@compnerd
Copy link
Member

compnerd commented Jun 3, 2025

FAILED: bin/swiftObservation.dll lib/swift/windows/aarch64/swiftObservation.lib 
C:\Windows\system32\cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_dll --intdir=stdlib\public\Observation\Sources\Observation\CMakeFiles\swiftObservation-windows-aarch64.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100226~1.0\x64\mt.exe --manifests  -- T:\5\bin\lld-link.exe /nologo stdlib\public\Observation\Sources\Observation\WINDOWS\aarch64\Observation.obj lib\swift\windows\aarch64\swiftrt.obj stdlib\public\Observation\Sources\Observation\CMakeFiles\swiftObservation-windows-aarch64.dir\ThreadLocal.cpp.obj  /out:bin\swiftObservation.dll /implib:lib\swift\windows\aarch64\swiftObservation.lib /pdb:bin\swiftObservation.pdb /dll /version:0.0 /INCREMENTAL:NO /OPT:REF /OPT:ICF /INCREMENTAL:NO -LIBPATH:T:\aarch64-unknown-windows-msvc\LLVM\.\lib   -LIBPATH:T:\aarch64-unknown-windows-msvc\Runtime\.\lib\swift\windows\aarch64   -LIBPATH:T:\5\bin\..\lib\swift\windows\aarch64   -LIBPATH:T:\5\bin\..\lib\swift\windows   -LIBPATH:\\usr\lib\swift   -LIBPATH:T:\aarch64-unknown-windows-msvc\Runtime\winsdk_lib_aarch64_symlinks lib\swift\windows\aarch64\swift_Concurrency.lib  lib\swift\windows\aarch64\swiftCore.lib  kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib && cd ."
LINK: command "T:\5\bin\lld-link.exe /nologo stdlib\public\Observation\Sources\Observation\WINDOWS\aarch64\Observation.obj lib\swift\windows\aarch64\swiftrt.obj stdlib\public\Observation\Sources\Observation\CMakeFiles\swiftObservation-windows-aarch64.dir\ThreadLocal.cpp.obj /out:bin\swiftObservation.dll /implib:lib\swift\windows\aarch64\swiftObservation.lib /pdb:bin\swiftObservation.pdb /dll /version:0.0 /INCREMENTAL:NO /OPT:REF /OPT:ICF /INCREMENTAL:NO -LIBPATH:T:\aarch64-unknown-windows-msvc\LLVM\.\lib -LIBPATH:T:\aarch64-unknown-windows-msvc\Runtime\.\lib\swift\windows\aarch64 -LIBPATH:T:\5\bin\..\lib\swift\windows\aarch64 -LIBPATH:T:\5\bin\..\lib\swift\windows -LIBPATH:\\usr\lib\swift -LIBPATH:T:\aarch64-unknown-windows-msvc\Runtime\winsdk_lib_aarch64_symlinks lib\swift\windows\aarch64\swift_Concurrency.lib lib\swift\windows\aarch64\swiftCore.lib kernel32.lib user32.lib gdi32.lib winspool.lib shell32.lib ole32.lib oleaut32.lib uuid.lib comdlg32.lib advapi32.lib /MANIFEST:EMBED,ID=2" failed (exit code 1) with the following output:
lld-link: error: could not open 'swiftWinSDK.lib': no such file or directory

[344/389][ 88%][361.882s] Linking CXX shared library bin\swiftWinSDK.dll

Seems like a bug with the Observation module's dependencies (cc: @phausler)

verify(Storage.str(), Flavor);
return finalize();
}

std::string ASTMangler::mangleDeclAsUSR(const ValueDecl *Decl,
StringRef USRPrefix) {
return (llvm::Twine(USRPrefix) + mangleAnyDecl(Decl, false)).str();
#ifndef NDEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of NDEBUG can you check if (CONDITIONAL_ASSERT_enabled())?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea. Thanks, Slava.

@compnerd
Copy link
Member

compnerd commented Jun 3, 2025

@swift-ci please test Windows platform

@ahoppen ahoppen force-pushed the dont-verify-mangle branch from ca2060f to ef95fda Compare June 4, 2025 10:45
@ahoppen
Copy link
Member Author

ahoppen commented Jun 4, 2025

@swift-ci Please smoke test

Verifying USR mangling adds ~30% overhead to indexing times. Since an incorrect USR mangling doesn't result in a correctness issue at the same level as a miscompile, save those 30% in non-assert builds.
@ahoppen ahoppen force-pushed the dont-verify-mangle branch from ef95fda to 563f971 Compare June 4, 2025 18:45
@ahoppen
Copy link
Member Author

ahoppen commented Jun 4, 2025

@swift-ci Please smoke test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 5, 2025

@swift-ci Please smoke test macOS

@ahoppen ahoppen merged commit ff483fe into swiftlang:main Jun 6, 2025
3 checks passed
@ahoppen ahoppen deleted the dont-verify-mangle branch June 6, 2025 06:36
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.

5 participants