Skip to content

Stop dumping stack for BadImageFormatException#8271

Merged
JaynieBai merged 2 commits intodotnet:mainfrom
danmoseley:fix.stack
Jan 6, 2023
Merged

Stop dumping stack for BadImageFormatException#8271
JaynieBai merged 2 commits intodotnet:mainfrom
danmoseley:fix.stack

Conversation

@danmoseley
Copy link
Copy Markdown
Member

Fixes #6224

Context

See issue -- a change was made to add more detail when RAR fails to resolve a reference due to encountering a BIFE. This added too much detail -- not only the message from the inner exception, but also the callstack. We should not dump callstack when there is no bug in MSBuild (or a task/logger).

Oddly enough I hit this again #8140 and didn't remember the original discussion, and therefore again assumed there was a bug.

Changes Made

  1. Fix BIFE to include the message from any inner exception. (There was a suggestion in the issue that it also include the exception type, but I don't know of any case where the type is necessary in order to disambiguate what happened, and we don't have precedent for doing it. It would also be ugly.)
  2. Revert the original change, now unnecesssary.

Testing

Added test that fails without the fix.

Notes

@danmoseley
Copy link
Copy Markdown
Member Author

cc @KirillOsenkov

@KirillOsenkov
Copy link
Copy Markdown
Member

Thanks for putting up a PR Dan! Agreed the stack trace looks like it's a bug in MSBuild.

@KirillOsenkov
Copy link
Copy Markdown
Member

Somehow I forgot that we'd discussed this last year and you've even provided a way to aggregate messages:
#6224 (comment)

@danmoseley
Copy link
Copy Markdown
Member Author

@Forgind does this look good to merge?

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 5, 2023
@Forgind
Copy link
Copy Markdown
Contributor

Forgind commented Jan 5, 2023

@Forgind does this look good to merge?

Looks good to me!

@danmoseley
Copy link
Copy Markdown
Member Author

could someone merge it then :)

@KirillOsenkov
Copy link
Copy Markdown
Member

My understanding is that since it's marked with the merge-when-branch-open label, magic will happen soon

@danmoseley
Copy link
Copy Markdown
Member Author

Oh! I see -- I should have figured that out. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RAR BadImageReferenceException needs to log the callstack and exception type

5 participants