-
Notifications
You must be signed in to change notification settings - Fork 676
Add QNX Probes config #4509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add QNX Probes config #4509
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
primiano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting. I have few very minor comments, but other than that LGTM.
Also note you need to sign the CLA in order to be able to contrib
| #if PERFETTO_BUILDFLAG(PERFETTO_OS_QNX) | ||
| int fd = sock_raw_.fd(); | ||
| int res = getpeereid(fd, &peer_uid_, nullptr); | ||
| gid_t peer_gid = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add a fallback gid_t to base/sys_types.h or this might break windows (well this one line is behind ifdef... but if you use it elsewhere)
| int res = getpeereid(fd, &peer_uid_, nullptr); | ||
| gid_t peer_gid = 0; | ||
| int res = getpeereid(fd, &peer_uid_, &peer_gid); | ||
| PERFETTO_CHECK(res == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a ignore_result(peer_gid); ? I'm surprised the compiler doesn't warn about it being unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added to be more explicit that we don't care about the result but i expect it has something to do with passing it to the getpeereid function as the compiler doesn't know if it is used internally due to external linkage
| TraceConfig test_config; | ||
| ConfigOptions opts; | ||
| opts.time = "2s"; | ||
| opts.categories.reserve(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? this is some code that is almsot never hit. this optimization here seems very debatable. This is by far NOT a perf-sensitive code.
I'd argue this it's not wroth neither the extra binary size, nor the extra line of scrolling while reading the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely fair points, but I am not trying to optimize the code here 😄. on QNX8.0 the emplace of the const char* was causing an -Warray-bounds error, where it wasn't in QNX7.1 this is most likely a bug in the gcc version we are using but it is causing the build to fail. This fix seemed to be the least invasive while suppressing the error. I am open to other options here as well if you have any, but if possible I would like to avoid disabling the warning on the whole project for QNX as it still provides useful utility.
Another option is to use push instead of emplace which doesn't seem to have this issue, and should provide the same performance as previously as the const char* will need to be cast to a std::string anyways. That being said I think no matter what solution we come up with here won't affect performance anyways as you mentioned this is almost never hit and will at most get hit once during the run AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand where the problem comes though. there is notthing bad doing push_back without reserve.
I am mainly curious if you have a minified repro on how that hits -Warray-bounds error
but also keep that as an aside, i'm okay keeping this, but curious what the warning is about as I can't see the problem here
43e1e87 to
eb6371c
Compare
|
|
||
| // Begin of protos/perfetto/config/qnx/qnx_config.proto | ||
|
|
||
| message QnxConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@primiano are you okay with QNX providing a description to the proto and a link to the traced_qnx_probes repo. I think having a link to the actual source code would provide very meaningful context to this proto.
If not here then it would be nice for a reference to QNX's traced_qnx_probes repo to exists within the Perfetto repo, so that it is possible to go deeper into QNX's integration of Perfetto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our side I think this is a great idea, but will wait for confirmation before integrating the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed change, leaving the solution generic so we won't need to update in case our branch name changes.
also still working on getting the CLA signed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@primiano are you okay with QNX providing a description to the proto and a link to the traced_qnx_probes repo. I think having a link to the actual source code would provide very meaningful context to this proto.
Yeah aboslutely. I think it's great to be able to see where this comes from.
eb6371c to
68db0c3
Compare
Copy the QNX Probes proto configuration so traced_qnx_probes can be used with the main perfetto project. This has been tested as working with traced_qnx_probes build from QNX repo and traced + perfetto build from this project As part of this commit all changes in order to get Perfetto compiling on QNX were ported. These changes allow for building on QNX7.1 and QNX8.0 The changes are as listed - Fix the toolchain to not specify a gcc version as QNX7.1 and QNX8.0 differ - Disable no-stringop-overflow for is_gcc in upd - in perfetto_cmd.cc prealloc vector to avoid -Warray-bounds for emplaced string this change won't affect the behaviour of the code but remove the error. - Add x86_64 support for QNX cross compiling. - Fix getpeereid to no longer take a NULL gid, this is because the change from iopkt to io-sock removes the ability to ignore NULL input args. No functional for iopkt but will fix segfault in libsocket for io-sock platforms.
68db0c3 to
b280b11
Compare
primiano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LG. land it whenever you sort out the CLA (I can't do anything about that)
Copy the QNX Probes proto configuration so traced_qnx_probes can be used with the main Perfetto project. This has been tested as working with traced_qnx_probes build from QNX Ports Perfetto repo and traced + perfetto build from this project
As part of this commit all changes in order to get Perfetto compiling on QNX were ported. These changes allow for building on QNX7.1 and QNX8.0 The changes are as listed