Page MenuHomePhabricator

Include inlined functions when figuring out a contiguous address range

Authored by aadsm on Apr 29 2019, 7:08 PM.



This diff changes the function LineEntry::GetSameLineContiguousAddressRange so that it also includes function calls that were inlined at the same line of code.

My motivation is to decrease the step over time of lines that heavly rely on inlined functions. I have multiple examples in the code base I work that makes a step over stop 20 or mote times internally. This can easly had up to step overs that take >500ms which I was able to lower to 25ms with this new strategy.

The reason the current code is not extending the address range beyond an inlined function is because when we resolve the symbol at the next address of the line entry we will get the entry line corresponding to where the original code for the inline function lives, making us barely extend the range. This then will end up on a step over having to stop multiple times everytime there's an inlined function.

To check if the range is an inlined function at that line I also get the block associated with the next address and check if there is a parent block with a call site at the line we're trying to extend.

To check this I created a new function in Block called GetContainingInlinedBlockWithCallSite that does exactly that. I also added a new function to Declaration for convinence of checking file/line named CompareFileAndLine.

To avoid potential issues when extending an address range I added an Extend function that extends the range by the AddressRange given as an argument. This function returns true to indicate sucess when the rage was agumented, false otherwise (e.g.: the ranges are not connected). The reason I do is to make sure that we're not just blindly extending complete_line_range by whatever GetByteSize() we got. If for some reason the ranges are not connected or overlap, or even 0, this could be an issue.

I also added a unit tests for this change and include the instructions on the test itself on how to generate the yaml file I use for testing.

Diff Detail


Event Timeline

aadsm created this revision.Apr 29 2019, 7:08 PM

Jim: this is a modified version of the patch we had talked about a while ago with needed fixes for making sure the inline function call site is the same as the starting file and line. We have iterated on this locally already, so I am good with this patch. Let us know what you think.

jingham requested changes to this revision.May 2 2019, 1:00 PM

This makes sense to me. Pinning the extension to the call site prevented all the cases I could think of where this might go wrong.

I had a bunch of small comments inline.

191–196 ↗(On Diff #197245)

The docs don't match the function signature.

112–113 ↗(On Diff #197245)

The argument is not called "rhs" anymore.

I'm not at all sure of the value of returning > < here. If you get > or < you have no way of knowing if that's because the files don't match, in which case so far as I can see the ordering is not useful, or if they are in the same file but with greater or lesser line number. To actually use the < or > you would have to go back and compare the files again anyway, so for all practical purposes you're really just returning a bool... And certainly that's how you are using it.

You were probably just copying the behavior from the function above, but that one doesn't make much sense either...

135 ↗(On Diff #197245)

are -> or

140 ↗(On Diff #197245)

We try to avoid default arguments unless there's a good reason, and in this case it looks like you pass the argument explicitly in almost all the uses, so I would make this a regular argument.

131 ↗(On Diff #197245)

intercept -> intersect

199 ↗(On Diff #197245)

start_call_site would be a clearer name for this. find_call_site doesn't say what it is.

60–87 ↗(On Diff #197245)

These defines don't seem specific to this test, is there a more general place you could put them?

96–122 ↗(On Diff #197245)

It looks like this bit of business has been copied around in a bunch of other tests in the unittest framework. Could we put this in a common place (like make a LLDBUnitTest : testing::Test class that does this?

193–194 ↗(On Diff #197245)

Can you make the return here not trivial (like return result + 5)} The way you have written the line return line doesn't contribute any code so you would step all the way out to the caller, but it would be good to test that we didn't just extend the range to our caller, i.e. that next on "int result ="... stops at the "return result" which it should do except in your example line 13 contributes no code.

279 ↗(On Diff #197245)

Add newline.

This revision now requires changes to proceed.May 2 2019, 1:00 PM
clayborg added inline comments.May 2 2019, 1:43 PM
113 ↗(On Diff #197245)

How about just returning a bool:

bool FileAndLineEqual(const Declaration &rhs);
140 ↗(On Diff #197245)

We tried that but the patch becomes a lot bigger if so. Happy to add all the other changes back, or we can make a new fucntion?

jingham added inline comments.May 2 2019, 2:06 PM
113 ↗(On Diff #197245)

That would be great. The whole "Compare" thing for Declarations doesn't make any sense to me. If you were trying to put Declarations in a container that required an ordering??? But that's not what this is for.

140 ↗(On Diff #197245)

Besides the declaration and definition, I only see 5 uses of GetSameLineContiguousAddressRange in lldb, and you've touched three of them. Am I missing something? If not, might as well change the other two.

aadsm marked an inline comment as done.May 2 2019, 2:16 PM
aadsm added inline comments.
112–113 ↗(On Diff #197245)

Oh, originally it was rhs but then I renamed and forgot to update the documentation.
Just to give some context: the reason I used the same interface was to provide a consistent API in the methods that do comparisons, but I see what you mean, I'll change it then.

aadsm marked 4 inline comments as done and an inline comment as not done.May 2 2019, 8:07 PM
aadsm added inline comments.
60–87 ↗(On Diff #197245)

I could put it in TestUtilities.h?

96–122 ↗(On Diff #197245)

How about adding a new function to TestUtilities.cpp named ReadYAMLObject?

193–194 ↗(On Diff #197245)

That's a good idea, thanks.

labath added a subscriber: labath.May 2 2019, 11:57 PM

I'm impressed by the test. I've tried to think of an alternative strategy for testing this (perhaps by adding a suitable mode to lldb-test), but I couldn't come up with one that a reasonable one, so this might be the best we can do right now. Thank you for taking the trouble to write that.

60–87 ↗(On Diff #197245)

I don't think most of these should exist actually... Instead of EXPECTED_NO_ERROR, you can use write if(std::error_code ec = ...) return llvm::errorCodeToError(ec);

For the rest, I'd suggest just inlining the macros and using llvm::createStringError instead of llvm::make_error<llvm::StringError>, as it's somewhat shorter (if you want, you can make a utility wrapper function around that to avoid the inconvertibleErrorCode argument).

89–94 ↗(On Diff #197245)

This is unused (and it wouldn't be right anyway).

96–122 ↗(On Diff #197245)

A utility function sounds nice. (a test class would be fine too, but I'd name it a bit less generic, as not all of our unit tests are in business of running yaml2obj and creating modules).

The part I'm not so sure about is the location. Originally the idea was that we would have a special subfolder for test helpers related to each module under test, but then at some point that got changed into TestingSupport which sounds more generic (this evolution here is visible in the fact that the cmake target name in that folder is called lldbUtilityHelpers). If we put this function there then we'd have to pull in the Core module (and everything that goes with it). Though that isn't that bad on it's own, it is a bit unfortunate, as right now the Utility unit test executable is our best defense against unexpected dependencies creeping into the main module. After this, that executable would link in the whole world again, and we'd lose this defense.

Another possibility might be to just put the yaml2obj (which is the main source of mess here) part in that file, and leave the construction of the Module object to the users.

150–152 ↗(On Diff #197245)

replace with ASSERT_THAT_EXPECTED(..., llvm::Succeeded()); (and in subsequent tests)

208 ↗(On Diff #197245)

The instructions aren't real (MachO obj2yaml does not preserve them), so it may be best to just leave them out to avoid confusion.

aadsm marked an inline comment as done.May 3 2019, 11:40 AM

@labath thank you for your kind words!

96–122 ↗(On Diff #197245)

Yeah, the way I did it locally was to create a function that only handles the yaml2obj part. Some tests use the file for other things other than creating a ModuleSpec.
I also put the responsibility of creating and cleaning up the object file in the caller. I was not sure how to handle the clean up of the temporary file for all different cases. Here's how it looks like from the caller side.

llvm::SmallString<128> obj;
if (auto ec = llvm::sys::fs::createTemporaryFile("source-%%%%%%", "obj", obj))
  return llvm::errorCodeToError(ec);
llvm::FileRemover obj_remover(obj);
if (auto error = ReadYAMLObjectFile("inlined-functions.yaml", obj))
  return llvm::Error(std::move(error));

What do you think?

aadsm updated this revision to Diff 198115.May 3 2019, 5:35 PM
  • Renamed CompareFileAndLine to FileAndLineEqual and made it return a bool
  • Made include_inlined_functions a mandatory parameter in GetSameLineContiguousAddressRange
  • Fix documentation
  • Refactored the object file creation from YAML
  • Got rid of a bunch of macros
  • Improved the test case
jingham accepted this revision.May 3 2019, 5:54 PM

LGTM, you should probably wait on Pavel's okay to the testing framework changes since that's more his baby...

This revision is now accepted and ready to land.May 3 2019, 5:54 PM
labath accepted this revision.May 5 2019, 11:14 PM

The test changes look fine to me. Thank you for doing that.

96–122 ↗(On Diff #197245)

Yeah, that's fine. I can think of a couple of ways of simplifying that further, but that would require making some tweaks to the FileRemover class (the class only has a handful of uses in llvm, so it's not surprising it has some rough edges). However, this is already a pretty big improvement over the current state, so thank you for doing that.

12 ↗(On Diff #198115)

What is this header used for? If need something to include to use createStringError, you should include llvm/Support/Error.h, as that's what defines it.

aadsm marked an inline comment as done.May 6 2019, 11:38 AM
aadsm added inline comments.
12 ↗(On Diff #198115)

I just forgot to remove it, I was using llvm::formatv before switching to createStringError.

aadsm updated this revision to Diff 198316.May 6 2019, 11:39 AM

Remove unused include

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 12:59 PM
rnk added a subscriber: rnk.May 6 2019, 1:35 PM

I think TestLineEntry.cpp didn't get added to the commit, so I removed it in r360076 to try to fix the build.