Page MenuHomePhabricator

Adds property to force enabling of GDB JIT loader for MacOS
ClosedPublic

Authored by yurydelendik on Feb 4 2019, 7:47 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yurydelendik created this revision.Feb 4 2019, 7:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 7:47 AM

Fix GetEnableLoaderForDarwin name

sunfish added a subscriber: sunfish.Feb 4 2019, 8:27 AM
jingham requested changes to this revision.Feb 4 2019, 11:57 AM
jingham added a subscriber: jingham.

It would be better to have the setting be an enum of "on/off/default", and then have the somebody - the current DynamicLoader plugin seems the best somebody - provide the default value if the setting hasn't been explicitly set. That way on any platform one could turn the loading on and off, which seems useful, and we wouldn't have to have a Darwin specific setting that will cease being applicable when the Darwin default switches.

This revision now requires changes to proceed.Feb 4 2019, 11:57 AM
  • Change to on/off/default property.

That looks good. Could you add a test for this setting to the ./functionalities/jitloader_gdb/TestJITLoaderGDB.py test? I wouldn't test that the default has any particular behavior because that might change over time. But test that if you turn it on, you do get load notifications, and if you turn it off you don't. Thanks!

labath added a subscriber: labath.Feb 5 2019, 3:35 AM

Do we need both this and the enable-jit-breakpoint setting? My impression was that the latter was meant to be used for disabling the gdb jit feature. Is the gdb plugin useful for anything if it does not set the breakpoint (i.e. enable = on, but enable-jit-bkpt = off). If it isn't, then could we just remove the latter?

  • Remove enable-jit-breakpoint option

Could you add a test for this setting to the ./functionalities/jitloader_gdb/TestJITLoaderGDB.py test?

Added tests

Do we need both this and the enable-jit-breakpoint setting? My impression was that the latter was meant to be used for disabling the gdb jit feature. Is the gdb plugin useful for anything if it does not set the breakpoint (i.e. enable = on, but enable-jit-bkpt = off). If it isn't, then could we just remove the latter?

Every time I tried to investigate that, I came to conclusion that it was just added as alternative for enable, but with intent to test plugin instantiation. Remoing in this patch. I can revert this change as needed.

Do we need both this and the enable-jit-breakpoint setting? My impression was that the latter was meant to be used for disabling the gdb jit feature. Is the gdb plugin useful for anything if it does not set the breakpoint (i.e. enable = on, but enable-jit-bkpt = off). If it isn't, then could we just remove the latter?

Every time I tried to investigate that, I came to conclusion that it was just added as alternative for enable, but with intent to test plugin instantiation. Remoing in this patch. I can revert this change as needed.

Thank you for looking into that. I think removing that setting is fine.

@jingham is this patch good to land?

jingham accepted this revision.Mar 4 2019, 10:26 AM

Yes, looks fine.

This revision is now accepted and ready to land.Mar 4 2019, 10:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2019, 6:23 AM

This is causing a failure on the Windows Bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/2299/steps/test/logs/stdio

It was broken because of another change which is why you probably didn't get a notification. Please, fix this quickly or revert the change, so that we don't end up with more failures that are not sending notifications because the bot's been broken all morning.