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

Mark asm code with no-executable-stack. #26

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

clalancette
Copy link

@clalancette clalancette commented Jul 20, 2023

Newer versions of binutils (2.39 and above) complain if you have assembly without explicitly marking that it is not using an executable stack (see
https://www.redhat.com/en/blog/linkers-warnings-about-executable-stacks-and-segments for more details). Mark it here to quiet that warning.

Newer versions of binutils (2.39 and above) complain
if you have assembly without explicitly marking that
it is not using an executable stack (see
https://www.redhat.com/en/blog/linkers-warnings-about-executable-stacks-and-segments
for more details).  Mark it here to quiet that warning.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
clalancette added a commit to ros2/mimick_vendor that referenced this pull request Jul 20, 2023
See ros2/Mimick#26 for more
information.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette marked this pull request as ready for review July 20, 2023 17:56
@clalancette
Copy link
Author

CI for this is in ros2/mimick_vendor#32 (comment)

@cottsay
Copy link
Member

cottsay commented Jul 20, 2023

This looks like something that should also be contributed upstream. Would it be appropriate to open a PR there as well?

@clalancette
Copy link
Author

This looks like something that should also be contributed upstream. Would it be appropriate to open a PR there as well?

Yes, agreed. I will go ahead and do that.

That said, we have a bunch of other patches here (which is why we still have this fork), so I think we'll need to take this PR anyway. Someday we should clean this up and submit all of our patches upstream.

@cottsay
Copy link
Member

cottsay commented Jul 20, 2023

...I think we'll need to take this PR anyway.

Agreed.

Someday we should clean this up and submit all of our patches upstream.

We can dream.

@cottsay
Copy link
Member

cottsay commented Jul 26, 2023

Do we ever build Mimick with non-GNU toolchains?

@clalancette
Copy link
Author

Do we ever build Mimick with non-GNU toolchains?

Good question. We do! Both MSVC and clang.

In the MSVC case, this definitely won't make a difference, as Mimick uses a different file for Windows (trampoline-x86_64-win.S).

However, there is some possibility this could be a problem with the clang build. Let's see what CI looks like there (and while we are at it, also RHEL). I've launched the builds in the other PR, just so the CI is all in one place: ros2/mimick_vendor#32 (comment)

@clalancette
Copy link
Author

It looks like both MSVC and Clang are OK with this change, so I think this (and ros2/mimick_vendor#32) can be reviewed again.

@cottsay
Copy link
Member

cottsay commented Jul 31, 2023

Here's the upstream PR for tracking purposes: Snaipe#38

@clalancette
Copy link
Author

Thanks! I'm going to go ahead and merge this one in, then fix up ros2/mimick_vendor#32 to point to the correct hash.

@clalancette clalancette merged commit 6b08fa3 into ros2 Aug 1, 2023
@delete-merged-branch delete-merged-branch bot deleted the clalancette/fix-executable-stack branch August 1, 2023 12:48
clalancette added a commit to ros2/mimick_vendor that referenced this pull request Aug 1, 2023
See ros2/Mimick#26 for more
information.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
clalancette added a commit to ros2/mimick_vendor that referenced this pull request Aug 1, 2023
See ros2/Mimick#26 for more
information.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@srmainwaring
Copy link

However, there is some possibility this could be a problem with the clang build.

Unfortunately breaks compilation on M1 macs.

[  4%] Building ASM object CMakeFiles/mimick.dir/src/asm/trampoline-aarch64.S.o
/Users/rhys/Code/ros2/rolling/ros2-rolling/src/Mimick/src/asm/trampoline-aarch64.S:80:19: error: unexpected token in '.section' directive
.section .note.GNU-stack, "", @progbits
% clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

srmainwaring added a commit to srmainwaring/Mimick that referenced this pull request Sep 22, 2023
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@clalancette
Copy link
Author

/Users/rhys/Code/ros2/rolling/ros2-rolling/src/Mimick/src/asm/trampoline-aarch64.S:80:19: error: unexpected token in '.section' directive

Oh, that's interesting. It works on aarch64 (gcc), it works on clang (amd64), but it fails on clang aarch64 (at least with that version of clang). Looking around, this probably has to do with the assembler on macOS: https://stackoverflow.com/questions/59799671/error-unexpected-token-in-section-directive-section-multiboot .

I'm open to ideas on what to do. I don't have an aarch64 macOS machine to test on, unfortunately.

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.

3 participants