This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] A small fix in GetBreakableLoadAddress() for MIPS
ClosedPublic

Authored by bhushan on Jan 11 2016, 1:21 AM.

Details

Reviewers
zturner
clayborg

Diff Detail

Repository
rL LLVM

Event Timeline

bhushan updated this revision to Diff 44438.Jan 11 2016, 1:21 AM
bhushan retitled this revision from to [LLDB][MIPS] A small fix in GetBreakableLoadAddress() for MIPS.
bhushan updated this object.
bhushan added a reviewer: clayborg.
bhushan set the repository for this revision to rL LLVM.

Summary:

Get the load address for the address given by 'symbol' and 'function'.
Earlier, this was done for 'function' only, this patch does it for 'symbol' too.

There's no test here.

clayborg accepted this revision.Jan 11 2016, 10:55 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Jan 11 2016, 10:55 AM
clayborg requested changes to this revision.Jan 11 2016, 10:56 AM
clayborg edited edge metadata.

Actually, can you add a test?

This revision now requires changes to proceed.Jan 11 2016, 10:56 AM
bhushan updated this revision to Diff 44723.Jan 13 2016, 2:32 AM
bhushan edited edge metadata.

This diff adds a testcase to test this patch.

The test gets all assembly instructions from the function and finds out the address of instruction in delay slot.
Then it tries to place a breakpoint on that address and verifies if breakpoint has been adjusted correctly.
The breakpoint should get placed on branch instruction instead of delay slot instruction.

In the future when you update the diff of a review, you need to base the diff against the original code, not against your first diff. So the new diff should be a superset of your old diff.

packages/Python/lldbsuite/test/lldbtest.py
1212–1225

You don't really need this, you can just use @skipIf(archs=not_in(['mips']))

bhushan updated this revision to Diff 44841.Jan 14 2016, 1:54 AM
bhushan updated this object.
bhushan added a reviewer: zturner.

Hi Zachary,

If we use @skipIf then the list would require to contain all possible MIPS variations and the list will grow long.
for ex: @skipIf(archs=not_in(['mips32','mips32r2', 'mips32r3', 'mips64','mips64r2', 'mips64r3', 'mips64r6' ......]))

@skipUnlessMips covers all these possible values using regular expression matching.

labath added a subscriber: labath.Jan 14 2016, 2:12 AM

Hi Zachary,

If we use @skipIf then the list would require to contain all possible MIPS variations and the list will grow long.
for ex: @skipIf(archs=not_in(['mips32','mips32r2', 'mips32r3', 'mips64','mips64r2', 'mips64r3', 'mips64r6' ......]))

@skipUnlessMips covers all these possible values using regular expression matching.

I agree with Zachary that we have too many decorators and we shouldn't be expanding their number, it's simply not sustainable. I see two options here:

  • add a getMipsArchitectures() function and then write archs=not_in(getMipsArchitectures())
  • add a not_regex() function and write archs=not_regex('mips.*')

How does that sound?

Hi Zachary,

If we use @skipIf then the list would require to contain all possible MIPS variations and the list will grow long.
for ex: @skipIf(archs=not_in(['mips32','mips32r2', 'mips32r3', 'mips64','mips64r2', 'mips64r3', 'mips64r6' ......]))

@skipUnlessMips covers all these possible values using regular expression matching.

I agree with Zachary that we have too many decorators and we shouldn't be expanding their number, it's simply not sustainable. I see two options here:

  • add a getMipsArchitectures() function and then write archs=not_in(getMipsArchitectures())
  • add a not_regex() function and write archs=not_regex('mips.*')

How does that sound?

I think adding not_ regex() sounds better option to me just because in future if MIPS adds another architecture variation then getMipsArchitectures() would require an update.

not_regex will look like this:

def not_regex(pattern):
return lambda x : not re.match(pattern, x)`

and python test file will use it as:

@skipIf(archs=not_regex('mips*'))

If Greg and Zachary also agrees then I will submit a patch for this.

zturner edited edge metadata.Jan 14 2016, 8:10 AM

Hi Zachary,

If we use @skipIf then the list would require to contain all possible MIPS variations and the list will grow long.
for ex: @skipIf(archs=not_in(['mips32','mips32r2', 'mips32r3', 'mips64','mips64r2', 'mips64r3', 'mips64r6' ......]))

@skipUnlessMips covers all these possible values using regular expression matching.

I agree with Zachary that we have too many decorators and we shouldn't be expanding their number, it's simply not sustainable. I see two options here:

  • add a getMipsArchitectures() function and then write archs=not_in(getMipsArchitectures())
  • add a not_regex() function and write archs=not_regex('mips.*')

How does that sound?

Another idea is to change the implementation of not_in to do the same thing that check_list_or_lambda does. i.e. Return a lambda that iterates each item of the argument and uses substring match instead of exact match. This way @skipIf(archs=not_in('mips')) would actually work.

Any of these 3 options sounds fine though.

We could just teach the standard decorators to detect the type of the "archs" variable and do the right thing based off of the type. In the handler code you could have:

retype = type(re.compile('hello, world'))
if isinstance(archs, list):
    # Do what we do now and check if the arch is in the list
elif isinstance(arg, basestring):
    # "archs" is a single architecture, just check it
elif (isinstance(archs, retype):
    # Handle regex correctly

We could also add support for passing a function in "archs" that takes a single argument that is the architecture name so we can do things with lambdas, etc. This should be detectable in the above check.

The basestring check might need to be modified for python 3.

Then the decorator usage can be:

@skipIf(archs=re.compile('mips.*'))

or

@skipIf(archs='mips64r2'))

I like the idea of adding a getMipsArchitectures() function as previously suggested:

add a getMipsArchitectures() function and then write archs=not_in(getMipsArchitectures())

bhushan updated this revision to Diff 45237.Jan 19 2016, 3:08 AM
bhushan updated this object.
bhushan edited edge metadata.

Addressed review comments.
Instead of adding new decorator, this patch modifies existing skipUnlessArch to detect the type of the "archs" variable and do the things according to the type.
This handles regular expressions as well.

The python test file then uses this decorator as @skipUnlessArch(archs=re.compile('mips*')) to skip any architectures other than mips.

clayborg requested changes to this revision.Jan 19 2016, 10:15 AM
clayborg edited edge metadata.

Just factor the cool archs stuff into a function that others can use and this is good to go.

packages/Python/lldbsuite/test/lldbtest.py
1047–1051

It would be nice to put this code that handles "archs" into a separate function:

def matchArchitectures(archs):
    # All code from above

Then have your code call it. This way other function can use this cool new functionality for testing archs since many many other skip decorator functions use "archs" in their arguments.

This revision now requires changes to proceed.Jan 19 2016, 10:15 AM
bhushan updated this revision to Diff 45364.Jan 20 2016, 1:27 AM
bhushan edited edge metadata.

As suggested by Greg, added new function matchArchitectures(archs) which handles "archs".
This function can be used by other decorator functions for testing "archs".

clayborg accepted this revision.Jan 20 2016, 9:47 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Jan 20 2016, 9:47 AM