This is an archive of the discontinued LLVM Phabricator instance.

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 ↗(On Diff #197371)

<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 ↗(On Diff #197371)

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 ↗(On Diff #197371)

Add a space after ==.

112 ↗(On Diff #197371)

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

135 ↗(On Diff #197371)

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

llvm/lib/Support/Unix/Path.inc
157 ↗(On Diff #197371)

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

161 ↗(On Diff #197371)

Add spaces after the commas in the argument list.

167 ↗(On Diff #197371)

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
110 ↗(On Diff #197371)

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
110 ↗(On Diff #197371)

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
111 ↗(On Diff #197875)

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

146 ↗(On Diff #197875)

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 ↗(On Diff #197875)

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...

160 ↗(On Diff #197875)

... 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
146 ↗(On Diff #197875)

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 ↗(On Diff #199115)

Minor nit: Add a space before the =.

160 ↗(On Diff #199115)

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
160 ↗(On Diff #199115)

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.