Hi Daniel,
I see a few problems here.
First of all, MTR already has "--valgrind-option" to pass parameters to valgrind, and "--tool" can be one of them. Wouldn't it be better to parse the array properly to check whether the tool was set? The new option introduces an alternative way to do the same thing, which means they can set different tools at the same time, which is just confusing.
Further, the new option cannot be used by itself now, it still requires something in "--valgrind-option", because otherwise MTR adds "--show-reachable", which does not work with anything but memcheck. At the very least show-reachable should be moved from default_valgrind_args to the block where other options for memcheck are set.
Finally, if we decide to do this, the callgrind logic needs to be cleaned up. It does not work anyway, as it sets the option --base=xxx which was apparently removed from valgrind years ago; and in a different place of the script it also sets trace-children, only for callgrind. Why is it so, probably nobody remembers.
My suggestion is instead
1) to use --valgrind-option for setting the tool (and other things if necessary);
2) if --tool is present among provided valgrind options, skip the logic which sets memcheck-specific options, otherwise preserve it;
3) keep --callgrind option for compatibility, but convert it into "--tool=callgrind" at the very beginning, and get rid of the rest of the special logic related to it.
I have no objections against exporting MTR_TEST_NAME if it's anyhow useful. I expect the use will be limited, since in a general case the server is not restarted between separate test runs, so updated values can only be picked up by the wrapper if mysqltest itself is run under valgrind. But maybe it will become handy for some other purposes.
Task
Major
Hi Daniel,
I see a few problems here.
First of all, MTR already has "--valgrind-option" to pass parameters to valgrind, and "--tool" can be one of them. Wouldn't it be better to parse the array properly to check whether the tool was set? The new option introduces an alternative way to do the same thing, which means they can set different tools at the same time, which is just confusing.
Further, the new option cannot be used by itself now, it still requires something in "--valgrind-option", because otherwise MTR adds "--show-reachable", which does not work with anything but memcheck. At the very least show-reachable should be moved from default_valgrind_args to the block where other options for memcheck are set.
Finally, if we decide to do this, the callgrind logic needs to be cleaned up. It does not work anyway, as it sets the option --base=xxx which was apparently removed from valgrind years ago; and in a different place of the script it also sets trace-children, only for callgrind. Why is it so, probably nobody remembers.
My suggestion is instead
1) to use --valgrind-option for setting the tool (and other things if necessary);
2) if --tool is present among provided valgrind options, skip the logic which sets memcheck-specific options, otherwise preserve it;
3) keep --callgrind option for compatibility, but convert it into "--tool=callgrind" at the very beginning, and get rid of the rest of the special logic related to it.
I have no objections against exporting MTR_TEST_NAME if it's anyhow useful. I expect the use will be limited, since in a general case the server is not restarted between separate test runs, so updated values can only be picked up by the wrapper if mysqltest itself is run under valgrind. But maybe it will become handy for some other purposes.