This is an archive of the discontinued LLVM Phabricator instance.

libsanitizer: Guard cyclades inclusion in sanitizer
ClosedPublic

Authored by tnfchris on May 7 2021, 2:36 AM.

Details

Summary

The Linux kernel has removed the interface to cyclades from
the latest kernel headers[1] due to them being orphaned for the
past 13 years.

libsanitizer uses this header when compiling against glibc, but
glibcs itself doesn't seem to have any references to cyclades.

Further more it seems that the driver is broken in the kernel and
the firmware doesn't seem to be available anymore.

As such since this is breaking the build of libsanitizer (and so the
GCC bootstrap[2]) I propose to remove this.

[1] https://lkml.org/lkml/2021/3/2/153
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100379

Diff Detail

Event Timeline

tnfchris requested review of this revision.May 7 2021, 2:36 AM
tnfchris created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2021, 2:36 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Alternative to this would be

#ifdef __has_include
# if __has_include(<linux/cyclades.h>)
#  include <linux/cyclades.h>
#  define SANITIZER_LINUX_CYCLADES 1
# endif
#endif

and use #ifdef SANITIZER_LINUX_CYCLADES to guard the remaining stuff.

Note, the current __has_include uses in sanitizers are incorrect:

sanitizer_common/sanitizer_platform.h:#if __has_include(<features.h>) && !defined(__ANDROID__)
sanitizer_common/sanitizer_platform_limits_netbsd.cpp:#if __has_include(<dev/iscsi/iscsi_ioctl.h>)
sanitizer_common/sanitizer_platform_limits_netbsd.cpp:#if __has_include(<netinet/ip_fil.h>)
sanitizer_common/sanitizer_mac.cpp:#if defined(__has_include) && __has_include(<os/trace.h>)
sanitizer_common/sanitizer_mac.cpp:#if defined(__has_include) && __has_include(<CrashReporterClient.h>)
tsan/rtl/tsan_interceptors_mac.cpp:#if defined(__has_include) && __has_include(<xpc/xpc.h>)
tsan/rtl/tsan_interceptors_mac.cpp:#if defined(__has_include) && __has_include(<xpc/xpc.h>)

If sanitizer relies on has_include support, then it shouldn't use defined(has_include) &&, because it doesn't help anyway, if has_include is not defined then the #if line will not parse properly.
And if it doesn't rely on
has_include, it should use the portable way, see https://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005finclude.html for details.

Alternative to this would be

#ifdef __has_include
# if __has_include(<linux/cyclades.h>)
#  include <linux/cyclades.h>
#  define SANITIZER_LINUX_CYCLADES 1
# endif
#endif

Ah, I wasn't aware of __has_include :), Will update the patch.

tnfchris updated this revision to Diff 344017.May 10 2021, 3:48 AM
tnfchris retitled this revision from libsanitizer: Remove cyclades from sanitizer to libsanitizer: Guard cyclades inclusion in sanitizer.
tnfchris edited the summary of this revision. (Show Details)

Update diff

jakubjelinek added inline comments.May 10 2021, 3:50 AM
compiler-rt/lib/sanitizer_common/sanitizer_platform.h
401 ↗(On Diff #344017)

Note, this doesn't define SANITIZER_LINUX_CYCLADES 0 if __has_include is not defined.

tnfchris updated this revision to Diff 344020.May 10 2021, 4:26 AM
tnfchris marked an inline comment as done.

The Linux kernel has removed the interface to cyclades from the latest kernel headers[1] due to them being orphaned for the past 13 years.

libsanitizer uses this header when compiling against glibc, but glibcs itself doesn't seem to have any references to cyclades.

Sounds like cyclades interceptors should just be deleted?

The Linux kernel has removed the interface to cyclades from the latest kernel headers[1] due to them being orphaned for the past 13 years.

libsanitizer uses this header when compiling against glibc, but glibcs itself doesn't seem to have any references to cyclades.

Sounds like cyclades interceptors should just be deleted?

I had that in the original patch, but I changed it because in principle there could be someone, somewhere that's using it with downstream fixes?
I don't have any strong opinions either way though and am happy to also remove them (i.e. restore patch back to version 1)

@MaskRay was that a you prefer it that way or?

@MaskRay was that a you prefer it that way or?

I would go for removing it. @eugenis ?

tnfchris updated this revision to Diff 345390.May 14 2021, 3:54 AM
tnfchris edited the summary of this revision. (Show Details)

reverted to diff that removed

Ping. Sorry for the early ping, but would be good to get bootstrap working again.

eugenis accepted this revision.May 19 2021, 1:26 PM

I'd be OK with removing it. 13 years should be enough time to deprecate features.

Context not available.

This change is simple enough so it does not matter, but please upload future diffs with full context.

This revision is now accepted and ready to land.May 19 2021, 1:26 PM

Ah I forgot the context.. Thanks @eugenis !

This revision was landed with ongoing or failed builds.May 20 2021, 3:07 AM
This revision was automatically updated to reflect the committed changes.
tnfchris reopened this revision.May 20 2021, 6:46 AM

Buildbot has shown a test failure in hwasan with reverting this. Investigating.

https://lab.llvm.org/buildbot/#/builders/53/builds/2778/steps/7/logs/FAIL__HWAddressSanitizer-aarch64__stack-uar-dynami

Not sure why removing cyclades causes a stack check to fail..

This revision is now accepted and ready to land.May 20 2021, 6:46 AM

The test HWAddressSanitizer-aarch64 :: TestCases/stack-uar-dynamic.c fails already at commit a647100b4320923b0e9d156cc3872b3be470ad98 which is before mine.
As such can't be caused by mine, I'll re-commit the patch.

This revision was automatically updated to reflect the committed changes.
justinkb added a subscriber: justinkb.EditedJul 2 2021, 2:58 AM

Any chance this gets backported to 10, 11? edit: let me rephrase that, if I backport this to 10 and 11, will you merge it?

Any chance this gets backported to 10, 11?

I don't think there are going to be any more releases for 10 or 11. But it might be interesting for 12.

Can anyone say if this is ABI-compatible? It looks like none of these headers are installed.

It certainly removes an API, but according to the commit message that hasn't usable for a while and it would be good to have the latest LLVM buildable with the latest glibc.

I believe it will be part of 12.0.1 already, luckily. Already ported to
release/12.x afaict