Skip to content

Chgans/allow preload injector override#543

Open
chgans wants to merge 3 commits into
KDAB:masterfrom
chgans:chgans/allow-preload-injector-override
Open

Chgans/allow preload injector override#543
chgans wants to merge 3 commits into
KDAB:masterfrom
chgans:chgans/allow-preload-injector-override

Conversation

@chgans

@chgans chgans commented Mar 19, 2019

Copy link
Copy Markdown
Contributor

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.

@akreuzkamp akreuzkamp left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

vkrause commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

chgans commented Jul 15, 2021

Copy link
Copy Markdown
Contributor Author

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

CLAassistant commented Jun 2, 2023

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@chgans

chgans commented Jun 3, 2023

Copy link
Copy Markdown
Contributor Author

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

@chgans

chgans commented Jun 3, 2023

Copy link
Copy Markdown
Contributor Author

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

winterz commented Jun 3, 2023

Copy link
Copy Markdown
Contributor

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