-
Notifications
You must be signed in to change notification settings - Fork 203
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
[NativeAOT-LLVM] Add WasmExternalDwarf property to create an external_debug_info section #2786
base: feature/NativeAOT-LLVM
Are you sure you want to change the base?
Conversation
…ace the DWARF in a separate file
Cc @dotnet/nativeaot-llvm |
Do we need to do something to add the |
It is a bit less pressing in upstream's case because the managed However, I foresee this may be tricky in the upstream's case because of the names section. The names section is used by the engines to print out stack traces on traps (unhandled exceptions, asserts, ...). It would have to be tested whether the browsers out there are eager to download this sidecar file (that will contain the names section stripped from the main file) to print these stack traces. I suspect not. In the NAOT's case this is not a big problem because we store the stack trace metadata directly in the executable, and are not super interested in native runtime stacks (since this 'native' part is small). |
@@ -35,6 +35,7 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
<IlcTreatWarningsAsErrors Condition="'$(IlcTreatWarningsAsErrors)' == ''">$(TreatWarningsAsErrors)</IlcTreatWarningsAsErrors> | |||
<_IsiOSLikePlatform Condition="'$(_targetOS)' == 'maccatalyst' or $(_targetOS.StartsWith('ios')) or $(_targetOS.StartsWith('tvos'))">true</_IsiOSLikePlatform> | |||
<_IsApplePlatform Condition="'$(_targetOS)' == 'osx' or '$(_IsiOSLikePlatform)' == 'true'">true</_IsApplePlatform> | |||
<WasmExternalDwarf Condition="'$(WasmExternalDwarf)' == ''and '$(_targetOS)' == 'browser' and '$(DotNetJsApi)' != 'true'">true</WasmExternalDwarf> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a documented property of this in the NAOT toolchain: StripSymbols
. We can use it.
Also, why disable for DotNetJsApi
? I think it makes sense for DotNetJsApi
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, I didn't really know, and just copied what was in the publish file for the .wasm
file. Will remove it.
|
||
<CustomLinkerArg Condition="'$(WasmExternalDwarf)' == 'true'" Include="-gseparate-dwarf" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<CustomLinkerArg Condition="'$(WasmExternalDwarf)' == 'true'" Include="-gseparate-dwarf" /> | |
<CustomLinkerArg Condition="'$(WasmExternalDwarf)' == 'true'" Include="-gseparate-dwarf" /> |
I think we should pass something like GetFilenameWithoutExtension($(NativeBinary)).debug.wasm
here to avoid the odd .wasm.debug.wasm
thing.
<WasmDwarfLinkerPath Condition="'$(EMSDK)' != ''">"$(EMSDK)/upstream/bin/llvm-dwp$(ExeExt)"</WasmDwarfLinkerPath> | ||
<WasmDwarfLinkerPath Condition="'$(EMSDK)' == ''">"$(EmscriptenSdkToolsPath)/bin/llvm-dwp$(ExeExt)"</WasmDwarfLinkerPath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like leftovers from DWP (here and above with DwarfObjects
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
Change extension to just `.debug.wasm` clear up old dwp stuff
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
Outdated
Show resolved
Hide resolved
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets
Outdated
Show resolved
Hide resolved
<Delete Files="$(PublishDir)\$(TargetName).debug.wasm" Condition="'$(_targetOS)' == 'browser' and '$(DotNetJsApi)' != 'true'"/> | ||
<Copy SourceFiles="$(NativeOutputPath)$(TargetName).debug.wasm" DestinationFolder="$(PublishDir)" Condition="'$(_targetOS)' == 'browser' and '$(DotNetJsApi)' != 'true'"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen for DotNetJsApi
as well and check whether the .debug.wasm
file is present to copy, ideally using $(WasmSymbolFile)
.
@@ -587,6 +591,7 @@ The .NET Foundation licenses this file to you under the MIT license. | |||
<CustomLinkerArg Include="-s TOTAL_STACK=$(IlcWasmStackSize)" /> | |||
<CustomLinkerArg Condition="'$(WasmEnableJSBigIntIntegration)' == 'true'" Include="-s WASM_BIGINT=1" /> | |||
<CustomLinkerArg Condition="'$(IlcLlvmExceptionHandlingModel)' == 'cpp'" Include="-s DISABLE_EXCEPTION_CATCHING=0" /> | |||
<CustomLinkerArg Condition="'$(StripSymbols)' == 'true'" Include="-gseparate-dwarf="$(WasmSymbolFile)"" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, does this embeds an absolute path, doesn't it. I think it would better to embed a relative path, i. e. just the file name. Otherwise I don't see it working even in our case where we copy from /native
to /publish
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, testing what this property does, this in fact the correct way to use it. Using an absolute path results in the relative path, and using a relative path result in something that is relative to the current working directory. Tricky but workable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow the outcome of the comment here. It is ok as is? I think it was working when I tested it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is ok as is?
Yes.
…e.targets Co-authored-by: SingleAccretion <[email protected]>
…e.targets Thanks Co-authored-by: SingleAccretion <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, exciting to see this! Could you check that our DotNetJs
test is still debuggable, just in case?
This PR adds a
WasmExternalDwarf
build property,true
by default forbrowser
that will add aexternal_debug_info
section and place the DWARF info a separate.debug.wasm
file.wasmtime
does not support this, at present, so only applies for the browser build.