- Notifications
You must be signed in to change notification settings - Fork215
[NativeAOT-LLVM] Add WasmExternalDwarf property to create an external_debug_info section#2786
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:feature/NativeAOT-LLVM
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ace the DWARF in a separate file
yowl commentedNov 19, 2024
Cc @dotnet/nativeaot-llvm |
pavelsavara commentedNov 19, 2024
Do we need to do something to add the |
SingleAccretion commentedNov 19, 2024
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). |
| <IlcTreatWarningsAsErrorsCondition="'$(IlcTreatWarningsAsErrors)' == ''">$(TreatWarningsAsErrors)</IlcTreatWarningsAsErrors> | ||
| <_IsiOSLikePlatformCondition="'$(_targetOS)' == 'maccatalyst' or $(_targetOS.StartsWith('ios')) or $(_targetOS.StartsWith('tvos'))">true</_IsiOSLikePlatform> | ||
| <_IsApplePlatformCondition="'$(_targetOS)' == 'osx' or '$(_IsiOSLikePlatform)' == 'true'">true</_IsApplePlatform> | ||
| <WasmExternalDwarfCondition="'$(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 forDotNetJsApi? I think it makes sense forDotNetJsApi 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.
| <CustomLinkerArgCondition="'$(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.
| <CustomLinkerArgCondition="'$(WasmExternalDwarf)' == 'true'"Include="-gseparate-dwarf" /> | |
| <CustomLinkerArgCondition="'$(WasmExternalDwarf)' == 'true'"Include="-gseparate-dwarf" /> |
I think we should pass something likeGetFilenameWithoutExtension($(NativeBinary)).debug.wasm here to avoid the odd.wasm.debug.wasm thing.
| <WasmDwarfLinkerPathCondition="'$(EMSDK)' != ''">"$(EMSDK)/upstream/bin/llvm-dwp$(ExeExt)"</WasmDwarfLinkerPath> | ||
| <WasmDwarfLinkerPathCondition="'$(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 withDwarfObjects).
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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| <DeleteFiles="$(PublishDir)\$(TargetName).debug.wasm"Condition="'$(_targetOS)' == 'browser' and '$(DotNetJsApi)' != 'true'"/> | ||
| <CopySourceFiles="$(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 forDotNetJsApi as well and check whether the.debug.wasm file is present to copy, ideally using$(WasmSymbolFile).
| <CustomLinkerArgInclude="-s TOTAL_STACK=$(IlcWasmStackSize)" /> | ||
| <CustomLinkerArgCondition="'$(WasmEnableJSBigIntIntegration)' == 'true'"Include="-s WASM_BIGINT=1" /> | ||
| <CustomLinkerArgCondition="'$(IlcLlvmExceptionHandlingModel)' == 'cpp'"Include="-s DISABLE_EXCEPTION_CATCHING=0" /> | ||
| <CustomLinkerArgCondition="'$(StripSymbols)' == 'true'"Include="-gseparate-dwarf="$(WasmSymbolFile)"" /> |
SingleAccretionNov 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.targetsCo-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
…e.targetsThanksCo-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
SingleAccretion left a comment
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 ourDotNetJs test is still debuggable, just in case?
This PR adds a
WasmExternalDwarfbuild property,trueby default forbrowserthat will add aexternal_debug_infosection and place the DWARF info a separate.debug.wasmfile.wasmtimedoes not support this, at present, so only applies for the browser build.