Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
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 | ||
|---|---|---|
| 1221–1234 ↗ | (On Diff #44723) | You don't really need this, you can just use @skipIf(archs=not_in(['mips'])) |
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.
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 correctlyWe 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())
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.
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 | ||
|---|---|---|
| 1040–1049 ↗ | (On Diff #45237) | It would be nice to put this code that handles "archs" into a separate function: def matchArchitectures(archs):
# All code from aboveThen 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. |
As suggested by Greg, added new function matchArchitectures(archs) which handles "archs".
This function can be used by other decorator functions for testing "archs".