This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Generalise MIPS arch names
ClosedPublic

Authored by nitesh.jain on Feb 2 2016, 10:10 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

nitesh.jain retitled this revision from to [LLDB][MIPS] Generalise MIPS arch names.
nitesh.jain updated this object.
nitesh.jain added a reviewer: jaydeep.
nitesh.jain set the repository for this revision to rL LLVM.
jaydeep accepted this revision.Feb 2 2016, 10:12 PM
jaydeep edited edge metadata.

Looks good to me

This revision is now accepted and ready to land.Feb 2 2016, 10:12 PM

Are you sure the triple keyword parameter supports this syntax? It doesn't
appear to be a regex since you're matching "mips*" instead of "mips.*". I
believe the regular string matching algorithm uses the python in keyword
(which does a substring check), so just saying triple = "mips" should
work I think.

Thanks zturner.

The triple is match using re.match(triple, lldb.DBG.GetSelectedPlatform().GetTriple()).
I will update diff with the suggested changes.

Update the diff with suggested changes

Looks fine to me

clayborg accepted this revision.Feb 3 2016, 10:24 AM
clayborg edited edge metadata.

I believe we recently switched "archs" so that we auto detect the type so you could use a regular expression. See above inlined comments.

Good to go unless you want to adopt any of my inlined suggestions.

packages/Python/lldbsuite/test/functionalities/thread/crash_during_step/TestCrashDuringStep.py
24 ↗(On Diff #46750)

You can use archs in this case if you want to by assigning it to a compiled regular expression:

@expectedFailureAll(archs=re.compile('^mips'))    # IO error due to breakpoint at invalid address
packages/Python/lldbsuite/test/functionalities/watchpoint/hello_watchlocation/TestWatchLocation.py
34 ↗(On Diff #46750)

Could use:

@expectedFailureAll(archs=re.compile('^mips'))
packages/Python/lldbsuite/test/functionalities/watchpoint/step_over_watchpoint/TestStepOverWatchpoint.py
76 ↗(On Diff #46750)

should this be:

if re.match("^mips",arch):

I know this is super confusing but expectedFailureAll doesn't yet support
regular expressions. I'm working on this code as we speak, so it will
soon. But what you're probably thinking of is skipUnlessArch (but
ironically not skipIfArch or expectedFailureArch). AHHHHHHHHH.

Anyway, that's why I'm fixing all this stuff, to make everything
consistent. For now if he's using expectedFailureAll (which is the
recommended way), none of the keyword arguments will take a regex.

Correction: If he's using expectedFailureAll, only the 'triple=' keyword
will take a regex.

nitesh.jain updated this revision to Diff 46913.Feb 4 2016, 7:07 AM
nitesh.jain updated this object.
nitesh.jain edited edge metadata.

Updated as per the suggestion.

nitesh.jain marked an inline comment as done.Feb 4 2016, 7:12 AM
nitesh.jain added inline comments.
packages/Python/lldbsuite/test/functionalities/thread/crash_during_step/TestCrashDuringStep.py
24 ↗(On Diff #46913)

Retain the triple checking logic . since expectedFailureAll doesn't yet support regular expressions for archs parameter.

packages/Python/lldbsuite/test/functionalities/watchpoint/hello_watchlocation/TestWatchLocation.py
34 ↗(On Diff #46913)

Retain the triple checking logic . since expectedFailureAll doesn't yet support regular expressions for archs parameter.

nitesh.jain updated this revision to Diff 46919.Feb 4 2016, 7:50 AM

updates diff with triple =re.compile('^mips') to check if architecture string starts with mips.

This revision was automatically updated to reflect the committed changes.