Skip to content

Conversation

@chgans
Copy link
Contributor

@chgans chgans commented Mar 19, 2019

This the PR for https://mail.kdab.com/pipermail/gammaray-interest/2019-March/000317.html.

It is untested so far, and i won't be able to test it before April. Nonetheless, you might want to comment on that.

Copy link
Contributor

@akreuzkamp akreuzkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You reuse --injector-override which is used by the gdb and lldb injectors to override the gdb/lldb executable. I don't know a lot about injection in general, but I wonder if it would make sense for the gdb/lldb injectors too, to allow overriding the probe-dll path. In that case it wouldn't be a good idea to reuse that commandline option. I think it's better to add a new one --probe-dll-override. @vkrause what do you think?

// and if so inject it before GammaRay
QStringList ldPreload;
foreach (const auto &lib, LibraryUtil::dependencies(exePath)) {
for (const auto &lib: LibraryUtil::dependencies(exePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do

const QVector<QByteArray> libraryDependencies = LibraryUtil::dependencies(exePath);
for (const auto &lib: libraryDependencies) {

else using range-for will detach the vector.

{
Q_UNUSED(probeFunc);

const QString actualProbeDll = m_probeDllOverride.isEmpty() ? probeDll : m_probeDllOverride;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better preserved at the caller side of this function, i.e. in Launcher::start. That requires the use of a separated commandline option though.

@akreuzkamp akreuzkamp requested a review from vkrause March 25, 2019 11:25
@vkrause
Copy link
Contributor

vkrause commented Mar 25, 2019

I think you are right, separating the command line arguments affecting the probe from those affecting the injector makes sense. The approach in general is fine IMHO, as discussed by email already.

@chgans
Copy link
Contributor Author

chgans commented Jul 15, 2021

Oops, forgot about this PR....
Came back here as i wanted to check if Android is supported, then look at what's cooking and found my own un-merged PR :)

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2023

CLA assistant check
All committers have signed the CLA.

@chgans
Copy link
Contributor Author

chgans commented Jun 3, 2023

I have signed NDA ages ago, and I have already contributed to GammaRay

@chgans
Copy link
Contributor Author

chgans commented Jun 3, 2023

When i follow the link, i get:


Error

There is no CLA to sign for KDAB/GammaRay

(could not find linked item for owner KDAB and repo GammaRay)

@winterz
Copy link
Contributor

winterz commented Jun 3, 2023

yes, I'm sorry. I'm trying to use cla-assistant webhook to make things easier than "paper". still learning how to use it and I need to pre-approved those who already signed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants