Skip to content

Add some optimization options to our asm #12

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.

Sign up for GitHub

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

Merged
cuviper merged 3 commits into from
May 29, 2020
Merged

Add some optimization options to our asm #12

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bb0fda5
Add some optimization options to our asm
nelhage May 27, 2020
2ed4e98
nonmem -> readonly, per code review
nelhage May 29, 2020
37d6ab5
Fix asm options for non-x86 platforms
nelhage May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/platform/systemtap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ macro_rules! sdt_asm(
#[doc(hidden)]
#[macro_export]
macro_rules! _sdt_asm(
($addr:tt, options $opt:tt, $provider:ident, $name:ident, $argstr:tt, $($arg:expr),*) => (
($addr:tt, options ($opt:tt), $provider:ident, $name:ident, $argstr:tt, $($arg:expr),*) => (
Copy link
Copy Markdown
Owner

@cuviper cuviper May 28, 2020

Choose a reason for hiding this comment

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

I think this change won't work for non-x86 targets that are passing just options() here. Maybe we should use a complete repetition pattern, options ( $( $opt:ident ),* ).

Copy link
Copy Markdown
Contributor Author

@nelhage nelhage May 29, 2020

Choose a reason for hiding this comment

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

You're right -- tested this on a non-x86 arch and it fails as you predict. Looking closer, though, do we actually need the att_syntax? If I remove it, it still builds for me on both 32- and 64-bit Linux x86, and as best I can tell everything we emit is either directives, which don't very with gas syntax, or a nop, which is perfectly valid Intel syntax. Removing that switch would certainly simplify things a bit.

Copy link
Copy Markdown
Owner

@cuviper cuviper May 29, 2020

Choose a reason for hiding this comment

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

It matters for how the register is written in the probe note. When I first tried in the default (intel) syntax, GDB wasn't able to parse the probe argument correctly. Maybe GDB (and other tools!) could be taught otherwise, but then we'd only work with newer versions.

asm!(concat!(r#"
990: nop
.pushsection .note.stapsdt,"?","note"
Expand All @@ -208,6 +208,6 @@ _.stapsdt.base: .space 1
"#
),
$(in(reg) (($arg) as isize) ,)*
options $opt,
options($opt, nomem, nostack, preserves_flags),
)
));