This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][cmake] Provide empty version of enable_execute_stack for baremetal targets
ClosedPublic

Authored by clm on May 9 2017, 3:28 PM.

Details

Summary

This patch allows compiler-rt to build for targets that don't have support for mprotect (ie. baremetal) by providing an empty version of enable_execute_stack.

Okay to commit?

Thanks,
Catherine

Diff Detail

Event Timeline

clm created this revision.May 9 2017, 3:28 PM
clm updated this revision to Diff 98360.May 9 2017, 3:40 PM

The first version of the patch failed to include the empty version of enable_execute_stack. The diff is now updated.

jroelofs requested changes to this revision.May 9 2017, 3:44 PM
jroelofs added inline comments.
lib/builtins/CMakeLists.txt
193

This blob of cmake goop would be simplified if, rather than having two files for enable_execute_stack{_empty,}.c, the flag just controlled the body's existence in enable_execute_stack.c.

lib/builtins/enable_execute_stack_empty.c
6 ↗(On Diff #98360)

LLVM style nit: we don't put a space between function names and the opening paren.

This revision now requires changes to proceed.May 9 2017, 3:44 PM
joerg added a subscriber: joerg.May 9 2017, 4:06 PM

Why do you call __enable_execute_stack in first place? I would just leave it and fix any potential callers.

Why do you call __enable_execute_stack in first place? I would just leave it and fix any potential callers.

GCC inserts calls to it for nested functions.

joerg added a comment.May 9 2017, 5:03 PM

To expand on what I said on IRC:

(1) I don't think anything should normally call it. For GCC, it depends on the target configuration whether it believes __enable_execute_stack is supported/required or not.
(2) We shouldn't fail to build compiler-rt for this.
(3) It is better to fail linking a user program with this when it is not expected / wasted code than silently hiding the problem.

As such, I'd prefer to just skip the whole default implementation for platforms where it shouldn't be used. This includes bare-metal.

clm updated this revision to Diff 98446.May 10 2017, 7:04 AM
clm edited edge metadata.

I've now updated the patch based on Joerg's comments. This version checks the setting of COMPILER_RT_BAREMETAL_BUILD before building enable_execute_stack.c.

clm planned changes to this revision.May 10 2017, 7:06 AM
joerg accepted this revision.May 10 2017, 7:36 AM
This revision was automatically updated to reflect the committed changes.