Skip to content
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

Disassembler inlining #494

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Disassembler inlining #494

wants to merge 3 commits into from

Conversation

lievenhey
Copy link
Contributor

this adds a simple detection for inlined code to elide it from the assembly view.

grafik

I am not sure if this works correctly so it would be nice if someone has a minimal example to test this.
This currently brakes syntax highlighting and code navigation

@lievenhey lievenhey force-pushed the disassembler-inlining branch from f7968a7 to 33ca598 Compare June 30, 2023 10:05
@lievenhey
Copy link
Contributor Author

lievenhey commented Jul 4, 2023

TODO: detect inline same file

@lievenhey lievenhey force-pushed the disassembler-inlining branch 2 times, most recently from 7d30963 to e654efd Compare July 4, 2023 11:32
@lievenhey
Copy link
Contributor Author

The inline detection is now working.

Output:
grafik

example.c:

#include <stdio.h>

inline int square(int i) {
        return i * i;
}

int main() {
        for (int i = 0; i < 10000; i++) {
                int sum = 0;
                for (int j = 0; j < i; j++) {
                        sum += square(j);
                }
                printf("Sum: %d\n", sum);
        }
}

@GitMensch
Copy link
Contributor

If it does what I think it does then: yay. Can you please share what the previous build outputs to be able to compare the results?

@lievenhey lievenhey force-pushed the disassembler-inlining branch from e654efd to e60f92b Compare July 5, 2023 13:28
@lievenhey
Copy link
Contributor Author

lievenhey commented Jul 5, 2023

before:
image
after:
image
(click to expand an entry)

@lievenhey lievenhey requested a review from milianw July 5, 2023 13:34
@lievenhey lievenhey force-pushed the disassembler-inlining branch 2 times, most recently from 00c117d to 0ca1838 Compare July 7, 2023 12:30
@lievenhey lievenhey force-pushed the disassembler-inlining branch from 0ca1838 to 31f8119 Compare July 25, 2023 15:17
this allows the disassembler to detect inlined functions and hide them
to clean up the disassembler
@lievenhey lievenhey force-pushed the disassembler-inlining branch from 31f8119 to 1bdcc71 Compare July 25, 2023 15:34
@GitMensch
Copy link
Contributor

Any idea why this fails on ArchLinux with clang?

@lievenhey
Copy link
Contributor Author

lievenhey commented Jul 28, 2023

no, seems to only happen on the ci
even if I ran the ci container locally, it fails to reproduce the issue

@lievenhey lievenhey force-pushed the disassembler-inlining branch from 4e1b1ad to b680ee4 Compare July 28, 2023 08:50
@lievenhey
Copy link
Contributor Author

found it
objdump ... on non clazy target:

00000000000012a0 <main>:
main():
/home/lieven/KDAB/hotspot/tests/test-clients/cpp-recursion/main.cpp:19

objdump ... on clazy target:

00000000000012f0 <main>:
std::basic_ios<char, std::char_traits<char> >::widen(char) const:
/opt/hotspot/tests/test-clients/cpp-recursion/main.cpp:19

I used the the first instance of something(): as function name. Since on clazy it is std::basic_ios<char, std::char_traits<char> >::widen(char) const it thinks main is an inlined function

time to rewrite it using libdwarf

@GitMensch
Copy link
Contributor

related to #495

@GitMensch
Copy link
Contributor

Would that need a doc update (screenshots, maybe)?
Is libdwarf a new dependency to be added?

@GitMensch
Copy link
Contributor

I'd like to give this a test - can you rebase that, please?

Apart from the actual inline view it will be interesting to see how the parsing with libdwarf effects the necessary time to open the disassembly view (while the time currently needed seems to be related to (the use of) Qt #505)

@lievenhey lievenhey marked this pull request as draft October 11, 2023 10:30
@GitMensch GitMensch mentioned this pull request Oct 12, 2023
@GitMensch
Copy link
Contributor

This will still take some time. We want to move the logic to the perfparser since it has all the logic for finding binaries etc.

Originally posted by @lievenhey in #518 (comment)

That sounds very reasonable. If I understand that correctly, this means that perfparser may use libdwarf to do this and more, and then perfparser will be used both for handling the actual "perf.data" (as now) and additionally do "something special" for the disassembly, but not replacing objdump, right?

@lievenhey
Copy link
Contributor Author

Disassembling will still be handled by objdump. We just use libdwarf for additional information.

@GitMensch
Copy link
Contributor

@lievenhey Is there a specific reason to keep this as a draft? If not, it would be nice to rebase and mark as "ready to merge" (actually a rebase would be nice in any case).

@lievenhey
Copy link
Contributor Author

Well for one it doesn't work that good and I hadn't much time to work on this.
We recently discussed using a real disassembly library so this will soon be out of date.
The reason is, that we are currently try to parse the human readable output of objdump, but that doesn't work so well with more complex information.

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.

2 participants