Conversation
valich
left a comment
valich
left a comment
There was a problem hiding this comment.
Please address the comments below. The code in gdb_wrapper is too cluttered with some technical consequences, too. Please fix.
lib/ruby-debug-ide/attach/util.rb
Outdated
| pids | ||
| end | ||
|
|
||
| def reset_port(argv) |
There was a problem hiding this comment.
I still don't get the purpose of that method. I feel uncomfortable with re-checking argv values once again when you have an appropriate library for that.
And also, mutating the parameter container is a bad practice.
There was a problem hiding this comment.
In any case argv is transferred to rdebug as a string in load_debugger method
lib/ruby-debug-ide/attach/util.rb
Outdated
| pid = q.pop | ||
| pids << pid | ||
|
|
||
| if(command_exists 'pgrep') |
There was a problem hiding this comment.
If you're checking for the command presence each time, does it mean that the command may disappear during children collection? If not, process such check beforehand and, ideally, also incapsulate running external processes into a separate method(s) in order to fix possible related bugs in a dedicated place.
There was a problem hiding this comment.
If the command does not exist, then only one element will be added to the queue and the check will be performed once. But it does not look very nice, I agree
lib/ruby-debug-ide/attach/util.rb
Outdated
| q = Queue.new | ||
| q.push(pid) | ||
|
|
||
| while(!q.empty?) do |
bin/gdb_wrapper
Outdated
| end | ||
| end | ||
| debugger.exit | ||
| pids.each_with_index do |pid, i| |
There was a problem hiding this comment.
Lots of nested methods deserve method extraction. Please split the code into sensible methods
bin/gdb_wrapper
Outdated
| debugger.exit | ||
| pids.each_with_index do |pid, i| | ||
|
|
||
| if(i == 1) |
There was a problem hiding this comment.
After the extraction of the logic below into methods this each_with_index and if i == ..., which are not beautiful, could be transformed to normal logic like "do something with the parent process and do something else with all its children" without any additional overhead.
bin/gdb_wrapper
Outdated
|
|
||
| debugger.attach_to_process | ||
| debugger.set_flags | ||
| attach_threads << Thread.new(argv) do |argv| |
There was a problem hiding this comment.
Another possible way to do this is to replace each with map and then just chain call the result.
bin/gdb_wrapper
Outdated
| DebugPrinter.print_debug("changing current uid from #{Process.uid} to #{options.uid}") | ||
| Process::Sys.setuid(options.uid.to_i) | ||
| end | ||
| debugger = choose_debugger(options.ruby_path, options.gems_to_include, debugger_loader_path, argv) |
There was a problem hiding this comment.
What is the purpose of removing pid as a instance variable of a NativeDebugger? Also, does it work at all if one attaches from multiple processes to one ruby executable?
valich
left a comment
valich
left a comment
There was a problem hiding this comment.
Much better, thanks. Still I have a couple of concerns :)
lib/ruby-debug-ide/attach/util.rb
Outdated
| pids = Array.new | ||
|
|
||
| if (!command_exists 'pgrep') | ||
| return pids |
There was a problem hiding this comment.
return [] unless command_exists
bin/rdebug-ide
Outdated
| opts.on("--attach-mode", "Tells that rdebug-ide is working in attach mode") do | ||
| options.attach_mode = true | ||
| end | ||
| opts.on("--attach-child", "child process for attach") do |
There was a problem hiding this comment.
I feel this explanation is not clear. If I understand the intent correctly, this param defines whether current process is somebody's child thus should not use port setting. Maybe --ignore-port?
bin/gdb_wrapper
Outdated
| end | ||
|
|
||
| argv = '["' + ARGV * '", "' + '"]' | ||
| argv = '["' + ARGV * '", "' |
There was a problem hiding this comment.
I think precalculating this part of argv (and creating such variable) is not really needed
| def attach_to_process(pid) | ||
| execute "attach #{pid}" | ||
| def attach_to_process | ||
| execute "attach #{@pid}" |
There was a problem hiding this comment.
Still I believe pid must be specified in executable when running gdb/lldb. AFAIR you've said yourself it does not work otherwise?
There was a problem hiding this comment.
For gdb your approach doesn't work without '-p' before the pid, so we have to use a different implementation for lldb and gbd. Therefore, it seems to me that it is better to leave it as it is
2 participants
No description provided.