Page MenuHomePhabricator

[compiler-rt] fix build on illumos
ClosedPublic

Authored by devnexen on Jul 17 2020, 9:49 AM.

Details

Summary
  • there are additional fields for glob_t struct thus size check is failing.
  • to access old mman.h api based on caddr_t, _XOPEN_SOURCE needs not to be defined.
  • prxmap_t constified.

Diff Detail

Event Timeline

devnexen created this revision.Jul 17 2020, 9:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2020, 9:49 AM
Herald added subscribers: Restricted Project, fedor.sergeev, dberris. · View Herald Transcript
devnexen added a reviewer: ro.Jul 17 2020, 9:57 AM
devnexen updated this revision to Diff 279064.Jul 18 2020, 11:59 PM
ro added a comment.Jul 22 2020, 5:21 AM

Thanks for doing this. It's a first step, but according to a quick test some time ago, much more will be needed. I expect there will be follow-up
patches to address those issues (missing support for ld --version-script, unconditional use of ld --as-needed, probably more)?

One point up front: please be more detailed and specific about which failures you're dealing with here. E.g. the comment about

old mman.h api based on caddr_t

is simply wrong: apart from failing to mention the affected function (madvise in my testing on OpenIndiana 2020.04), the problem is that
madvise isn't declared at all if XOPEN_SOURCE is defined.

Solaris 10 had the same problem, but at some point int the Solaris 11 cycle the prototype was made visible if __EXTENSIONS__ is defined,
just as expected. Independent of the current patch Illumos may want to do this, too.

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_solaris.cpp
205–206

Please remove the "non posix" part: POSIX always allows for
additional fields. The point is that they differ between Solaris
and Illumos.

I guess "There are additional fields that we are not interested in."
would be better.

compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
27

If this goes in at all, it needs to be before the various sanitizer_*.h includes above: they can include system headers, too.

In fact, I'm quite unhappy about this: it seems a terribly large hammer to get just a single missing declaration, and I fear there will be undesired side effects now or in the future.

It seems way easier to just declare madvise like

#if SANITIZER_SOLARIS
// Illumos' declaration of madvise cannot be made visible if _XOPEN_SOURCE is
// defined as g++ does on Solaris.
extern "C" int madvise(void *, size_t, int);
#endif

just before ReleaseMemoryPagesToOS. This works on both Illumos and Solaris and the risk is far lower.

AFAIK there's currently no way to distinguish between Illumos and Solaris at compile time right now. That would allow introduction of code that's needed by one but harms the other.

devnexen updated this revision to Diff 279806.Jul 22 2020, 6:52 AM
devnexen edited the summary of this revision. (Show Details)

Changes per feedback :
madvise prototype for access.

ro accepted this revision.Jul 22 2020, 7:04 AM

LGTM, thanks.

This revision is now accepted and ready to land.Jul 22 2020, 7:04 AM
This revision was automatically updated to reflect the committed changes.
ro added a comment.Jul 23 2020, 12:42 AM

Unfortunately I hadn't noticed that you'd silently changed the madvise prototype from the version I'd suggested (and tested on both Solaris 11.4 and OpenIndiana) to one using caddr_t instead of void * for the first arg, this way breaking the Solaris build. Now fixed in D84388.

Please don't do something like this: it just creates unnecessary work.

This is exactly the reason why I asked for a way to distinguish between Illumos and Solaris at compile time (e.g. by predefining __illumos__).
Fixing one by breaking the other just isn't an option.

devnexen added a comment.EditedJul 23 2020, 1:48 AM
In D84046#2168732, @ro wrote:

Unfortunately I hadn't noticed that you'd silently changed the madvise prototype from the version I'd suggested (and tested on both Solaris 11.4 and OpenIndiana) to one using caddr_t instead of void * for the first arg, this way breaking the Solaris build. Now fixed in D84388.

Please don't do something like this: it just creates unnecessary work.

This is exactly the reason why I asked for a way to distinguish between Illumos and Solaris at compile time (e.g. by predefining __illumos__).
Fixing one by breaking the other just isn't an option.

Alright apologies I copied/pasted the man page. I agree with additional illumos preprocessor constant, would be handy for some situations...