Page MenuHomePhabricator

Fixes for builds that require strict X/Open and POSIX compatiblity
ClosedPublic

Authored by daltenty on Apr 30 2019, 10:54 AM.

Details

Summary
  • use alternative to MAP_ANONYMOUS for allocating mapped memory if it isn't available
  • Use strtok_r instead of strsep as part of getting program path
  • Don't try to find the width of a terminal using "struct winsize" and TIOCGWINSZ on POSIX builds. These aren't defined under POSIX (even though some platforms make them available when they shouldn't), so just check if we are doing a X/Open or POSIX compliant build first.

Diff Detail

Repository
rL LLVM

Event Timeline

daltenty created this revision.Apr 30 2019, 10:54 AM
hubert.reinterpretcast requested changes to this revision.May 1 2019, 9:32 PM

These are defined under POSIX

Do you mean that these aren't defined under POSIX?

llvm/lib/Support/Unix/Memory.inc
94–106

<ins>On</ins><del>on</del> platforms that have it<ins>,</ins> we can use MAP_ANONYMOUS to get a memory<ins>-</ins>mapped page without file backing, but we need a fallback <del>by</del><ins>of</ins> opening /dev/zero for strictly POSIX platforms instead<ins>.</ins>

97

I would rather have fd assigned separately on each branch of the #if than to have the rest of the statement inside conditional inclusion blocks.

102

Add a space after ==.

108

To be consistent with the ifdef, this should be #elif defined(MAP_ANON).

133–134

The file descriptor returned by open and stored in fd is not closed on this path.

llvm/lib/Support/Unix/Path.inc
155–156

Once assigned, s always has the same value as pv. Do we need s?

166

Add spaces after the commas in the argument list.

172

Add spaces after the commas in the argument list.
Add space after while (or at least that is what clang-format does for me).

This revision now requires changes to proceed.May 1 2019, 9:32 PM
MaskRay added inline comments.
llvm/lib/Support/Unix/Memory.inc
107–111

I think you can just keep MAP_ANON and remove MAP_ANONYMOUS. Though marked as deprecated in Linux manpages, it is most widely used. It is also used by compiler-rt.

MaskRay added inline comments.May 1 2019, 10:56 PM
llvm/lib/Support/Unix/Memory.inc
107–111

You may need to rebase. I've deleted MAP_ANONYMOUS.

daltenty edited the summary of this revision. (Show Details)May 2 2019, 10:20 AM
daltenty updated this revision to Diff 197875.May 2 2019, 2:55 PM
  • Address formatting comments
  • Revert to using MAP_ANON
  • Close the file descriptor on all return paths in Memory::allocateMappedMemory
  • Cleanup the pointers we keep in getprogpath
daltenty marked 10 inline comments as done.May 2 2019, 2:57 PM
llvm/lib/Support/Unix/Memory.inc
110

I think using MMFlags |= MAP_ANON; will help readability/maintainability.

144

If it's okay to close the file here, then it is okay to close the file right after the call to mmap before checking whether mmap failed. We would only need to add one call to close (instead of three).

llvm/lib/Support/Unix/Path.inc
156–157

If we're going to move where the pv, etc. is declared, then we might as well move the declaration of s to this line...

166

... and declare t in the if:

if (char *t = strtok_r(s, ":", &state)) {
daltenty marked 3 inline comments as done.May 10 2019, 6:08 PM
daltenty added inline comments.
llvm/lib/Support/Unix/Memory.inc
144

If we close right after mmap, that would technically render the value of errno undefined or clobber it, and we may need to inspect it if the mmap failed (and according to spec we should only manipulate it if mmap failed)

daltenty updated this revision to Diff 199115.May 10 2019, 6:10 PM
llvm/lib/Support/Unix/Path.inc
156–157

Minor nit: Add a space before the =.

166

Does this compile?

daltenty updated this revision to Diff 199522.May 14 2019, 2:59 PM
daltenty marked 2 inline comments as done.

Fix style

daltenty updated this revision to Diff 199673.May 15 2019, 1:37 PM
  • Fix loop on strtok_r
daltenty marked an inline comment as done.May 15 2019, 1:37 PM
daltenty added inline comments.
llvm/lib/Support/Unix/Path.inc
166

Yes it does. Can you clarify?

This revision is now accepted and ready to land.May 15 2019, 3:19 PM
This revision was automatically updated to reflect the committed changes.