Page MenuHomePhabricator

[lldb/test] Make "inline" tests handle multiple statements at the same location

Authored by labath on Thu, May 7, 4:11 AM.



The test machinery translates each continuous block of "//%" comments
into a single breakpoint. If there's no code between the blocks the
breakpoints will end up at the same location in the program. When the
process stops at a breakpoint lldb correctly reports all breakpoint IDs,
but the test machinery only looks at the first one. This results in a
very dangerous situation as it means some checks can be silently

This patch fixes that by making the test machinery iterate through all
breakpoints at a given location and execute all commands.

Diff Detail

Event Timeline

labath created this revision.Thu, May 7, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, May 7, 4:11 AM
labath updated this revision to Diff 262614.Thu, May 7, 4:33 AM
  • disable a check that fails now. AFAICT, this never worked.
labath marked an inline comment as done.
labath added inline comments.

Raphael: This check wasn't executed before this patch. As far as I can tell, this functionality hasn't worked since the test was first introduced (r365698).

teemperor accepted this revision.Thu, May 7, 5:21 AM

Will this also fix these problems?

int i = 0;
//% code_that_is_executed

//% that that isn't executed as it's behind an empty line :(

I think disabling the check is fine. You can also just change the '= 14' to a '= 11'. I originally only intended to capture the current behavior and assumed we prefer local variables, but the agreement back then was that we don't know what is the correct behavior in this situation.

This revision is now accepted and ready to land.Thu, May 7, 5:21 AM

Changing might actually be a better idea as this way we at least know this isn't crashing.

shafik added a subscriber: shafik.Thu, May 7, 9:27 AM

Thank you! I am pleasantly surprised there was only one hidden broken test.

vsk added a comment.Thu, May 7, 10:27 AM

Thanks! LGTM, modulo any comments Raphael may have about the test change.


nit: I think this leaves the 'id' builtin defined as the last breakpoint id after the loop is done, maybe name it something else ('breakpoint_id'?).

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.

Changing might actually be a better idea as this way we at least know this isn't crashing.

Ok, I'll change the expectation then.


yes, of course.