Page MenuHomePhabricator

Make --thread option optional (MI)
ClosedPublic

Authored by ki.stfu on Jan 29 2015, 2:28 PM.

Details

Reviewers
abidh
Summary

Hello,

This patch do --thread option optional (according to docs).
Currently it is marked as "mandatory" option and it conflicts with official docs.

Also this patch includes following changes:

  • add test for -data-disassemble
  • add tests for -exec-next/-exec-next-instruction/-exec-step/-exec-step-instruction/-exec-finish commands
  • improve test program (a.c, b.c, main.c)
  • fix TestMiBreakpoint.py and TestMiSyntax.py (it's required after previous item)
  • add tests for -stack-info-depth and -stack-list-frames

Diff Detail

Event Timeline

ki.stfu updated this revision to Diff 18989.Jan 29 2015, 2:28 PM
ki.stfu retitled this revision from to Make --thread option optional (MI).
ki.stfu updated this object.
ki.stfu edited the test plan for this revision. (Show Details)
ki.stfu added a reviewer: abidh.
ki.stfu set the repository for this revision to rL LLVM.
ki.stfu added subscribers: Unknown Object (MLST), abidh.
abidh edited edge metadata.Feb 2 2015, 3:45 AM

Some comments:

Please make one patch that is focussed on one problem. Unrelated changes should be in a separate patch. This way it is easy to review/commit. Also it is easy if it has to be reverted later. I think I can take care of this patch by committing related changes together. But please take care of it in future.

The changes to make the --thread optional looks valid to me. I think they can go in now.

The changes in test cases may need a bit more work though. The use of 0 as start and end address in -data-disassemble does not look good. Some stepping tests are timing out for me so need more investigation. Also I dont understand what the changes in a.c and b.c are supposed to achieve. Can you describe in a few lines.

Thanks,
Abid

abidh added a comment.Feb 2 2015, 5:27 AM

zturner,
Should I close this now? Main part of the patch has been accepted and committed. Rest is unrelated change which should come in as a separate patch.

Thanks,
Abid

Hello Abid,

Thanks for committing.
OK. I'll submit the tests's improvements in another patch and create separate review for it.

abidh accepted this revision.Feb 2 2015, 6:14 AM
abidh edited edge metadata.
This revision is now accepted and ready to land.Feb 2 2015, 6:14 AM
abidh closed this revision.Feb 2 2015, 6:14 AM

Main part has been committed. Rest will be in a separate revision.