Page MenuHomePhabricator

[lldb] Set the correct triple when creating an ArchSpec for windows
ClosedPublic

Authored by wallace on Sep 6 2016, 4:33 PM.

Details

Summary

The function SetArchitecture, in the case of windows archs, unsets all the parameters of the triple and thus the arch becomes ELF, which is the default.
This change fixes this issue.

Diff Detail

Event Timeline

wallace updated this revision to Diff 70488.Sep 6 2016, 4:33 PM
wallace retitled this revision from to [lldb] Set the correct triple when creating an ArchSpec for windows.
wallace updated this object.
wallace added a reviewer: zturner.
wallace set the repository for this revision to rL LLVM.
wallace added a reviewer: sas.
wallace added a subscriber: Restricted Project.
zturner accepted this revision.Sep 6 2016, 4:38 PM
zturner edited edge metadata.

Does this affect the running of the test suite in any way? Is anything fixed or regressed as a result of this?

This revision is now accepted and ready to land.Sep 6 2016, 4:38 PM

BTW, you will need to rebase this on top of trunk, and make sure you run clang-format before submitting.

This fixes some issues with arm architectures (where this was unsetting the thumbv7 and was fallbacking to thumb).
This is also needed for https://reviews.llvm.org/D24284, where the objet file requires a correct architecture inferred from the machine arch

This fixes some issues with arm architectures (where this was unsetting the thumbv7 and was fallbacking to thumb).
This is also needed for https://reviews.llvm.org/D24284, where the objet file requires a correct architecture inferred from the machine arch

Right, I'm just wondering if when you run ninja check-lldb before and after this patch, do the results differ at all?

oh, thank you, first time i'm sending lldb diffs

wallace updated this revision to Diff 71226.Sep 13 2016, 1:01 PM
wallace updated this object.
wallace edited edge metadata.

rebase

I've run the tests and saw that two tests have been fixed
before:

Test Methods:       1829
Reruns:                0
Success:            1061
Expected Failure:    113
Failure:               0
Error:                 7
Exceptional Exit:      0
Unexpected Success:   11
Skip:                637
Timeout:               0
Expected Timeout:      0

After

Test Methods:       1829
Reruns:                0
Success:            1061
Expected Failure:    115
Failure:               0
Error:                 7
Exceptional Exit:      0
Unexpected Success:    9
Skip:                637
Timeout:               0
Expected Timeout:      0

So Expected Failures increased by two

I've run the tests and saw that two tests have been fixed
before:

Test Methods:       1829
Reruns:                0
Success:            1061
Expected Failure:    113
Failure:               0
Error:                 7
Exceptional Exit:      0
Unexpected Success:   11
Skip:                637
Timeout:               0
Expected Timeout:      0

After

Test Methods:       1829
Reruns:                0
Success:            1061
Expected Failure:    115
Failure:               0
Error:                 7
Exceptional Exit:      0
Unexpected Success:    9
Skip:                637
Timeout:               0
Expected Timeout:      0

So Expected Failures increased by two

Actually that means two tests have been broken :) Before they were succeeding even though we thought they should fail, and now they are failing as we would expect them to do. I'm not sure if this is good or bad. :) Is there any way you could run the test suite twice, once with your patch and once without, and see which two tests it is? And maybe post the log files of the two tests now that they are failing?

Hey, apparently this change doesn't affect the test coverage, there are at least two flaky tests that sometimes produce unexpected success: test_step_over_dwo, test_process_interrupt_dwo

I repeatedly ran the tests with and without my change it the flakiness was quite consistent

I can believe that :) Looks good then.

wallace added a comment.EditedSep 13 2016, 4:13 PM

btw, i don't have commit access. Can you commit it or should i request access?

I will commit it either later today or tomorrow.

Thanks for the reminder. Will do this today.

This revision was automatically updated to reflect the committed changes.