-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add Linux NativeAOT debug info regression test #81612
Conversation
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThe test runs llvm-dwarfdump and verifies that the number of warnings and errors is expected. As we improve the debug information we can start tracking individual warnings and errors, hopefully bringing this number to zero.
|
a0a4166
to
a2b6e02
Compare
The test runs llvm-dwarfdump and verifies that the number of warnings and errors is expected. As we improve the debug information we can start tracking individual warnings and errors, hopefully bringing this number to zero.
a2b6e02
to
52e235e
Compare
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
@dotnet/illink-contrib for review |
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 good otherwise!
Console.WriteLine($"Found {count} warnings and errors, expected between {MinWarnings} and {MaxWarnings}"); | ||
Console.WriteLine("This is likely a result of debug info changes. To see the new output, run the following command:"); | ||
Console.WriteLine("\tllvm-dwarfdump --verify " + Environment.ProcessPath); | ||
return 1; |
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.
Would it be possible to have this print the stdout and stderr of the --verify
we just ran so that we can see the detail of the problem?
I'm just imagining myself in the shoes of the person seeing this failure and saying "great, do I need to spin up a Linux machine to get a repro of this?".
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.
Might also be useful to print the count
all the time so that if it fails, one can go to a different PR and see if the count already was very close to the threshold and the fix is to just update the range.
Basically, if I can fix a test failure by looking at logs, I'm happier than if I have to repro the failure locally first.
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.
I think output the log makes a ton of sense -- but only when the log is smaller than 50000 lines. I'm happy to make that change when we get the warnings down to a reasonable number.
Always printing the count is a great idea, though.
|
||
proc = Process.Start(new ProcessStartInfo | ||
{ | ||
FileName = "/bin/sh", |
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.
Why do we wrap this in /bin/sh
and don't wrap the --version
one before?
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.
Oops. This was a remnant from when I was using grep
to do the filtering, instead of doing it directly in C#. I'll remove.
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
The test runs llvm-dwarfdump and verifies that the number of warnings and errors is expected. As we improve the debug information we can start tracking individual warnings and errors, hopefully bringing this number to zero.