Skip to content
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

Remove --flat from default dsymutil options #89358

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Conversation

agocke
Copy link
Member

@agocke agocke commented Jul 22, 2023

--flat is a diagnostic-only option and not fully supported. It can still be specified if necessary through the DsymUtilOptions property and NativeSymbolExt property, but it will no longer be the default.

--flat is a diagnostic-only option and not fully supported. It
can still be specified if necessary through the DsymUtilOptions
property and NativeSymbolExt property, but it will no longer be
the default.
@ghost
Copy link

ghost commented Jul 22, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

--flat is a diagnostic-only option and not fully supported. It can still be specified if necessary through the DsymUtilOptions property and NativeSymbolExt property, but it will no longer be the default.

Author: agocke
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@jkotas jkotas requested a review from filipnavara July 23, 2023 03:45
@jkotas
Copy link
Member

jkotas commented Jul 23, 2023

I assume that this is related to #86734.

Are we going to need a follow up change to upload symbols in the dsym format for AOT compiled binaries in the official builds?

We have been using --flat option for unmanaged binaries

set(DSYMUTIL_OPTS "--flat")
. Should we remove the --flat option for the unmanaged binaries as well?

@filipnavara
Copy link
Member

Should we remove the --flat option for the unmanaged binaries as well?

There's a tracking issue for that - #88286.

I would be happy if this PR goes in since it makes debugging apps much easier but there's always the concern about SymStore and related utilities that are not able to handle dSYM yet.

@agocke
Copy link
Member Author

agocke commented Jul 24, 2023

Are we going to need a follow up change to upload symbols in the dsym format for AOT compiled binaries in the official builds?

Yeah, we can either set the --dwarf flag just for our CI, or we can support zipping up the dsym bundle. I'd prefer the latter, but will see if I can do the former in this PR just to unblock things.

As for native builds, I think they should also use the dsym bundle eventually, but it matters slightly less since it's only for debugging coreclr itself, which is a more uncommon user scenario. In this case, debugging Mac-compiled Native AOT apps is a first-class scenario, so we should use the Apple-preferred thing by default.

@agocke
Copy link
Member Author

agocke commented Jul 25, 2023

Well, I looked in the crossgen2 symbols package and I actually don't see the .dwarf file in there. Is that expected, @hoyosjs?

@hoyosjs
Copy link
Member

hoyosjs commented Jul 25, 2023

No, that'd break postmortem of crossgen. There's several problems. Right now the easiest one is telling the SDK that dsym is a symbol file, after you need to teach the uploaders how to index dsyms.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

This change looks good on its own. It would be nice to have the follow up plan figured out before merging.

@agocke
Copy link
Member Author

agocke commented Jul 25, 2023

@hoyosjs can the symbol uploader index .dwarf files today? Also, .dsym is a folder. Would we want to zip it?

@hoyosjs
Copy link
Member

hoyosjs commented Jul 25, 2023

I thought it was an archive. Don't know of prior art in symbol packages, need to check

@agocke
Copy link
Member Author

agocke commented Aug 9, 2023

OK, looking into it, I'm pretty sure the existing system is not working for collecting ILC symbols. But, to avoid any regressions, I've special-cased crossgen2 to have the old behavior.

Long-term, I don't see any fundamental restrictions to using .dsym files everywhere. We should just make sure that things up the chain are happy with it.

@agocke agocke merged commit 44e63d4 into dotnet:main Aug 10, 2023
116 checks passed
@agocke agocke deleted the no-sym-export branch August 10, 2023 05:46
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants