This is an archive of the discontinued LLVM Phabricator instance.

Add clang headers that fix machine-dependent definitions on FreeBSD 9.2 (2nd attempt)
AbandonedPublic

Authored by kutuzov.viktor.84 on Jul 22 2014, 12:20 PM.

Details

Reviewers
theraven
rsmith
Summary

This patch is an updated version of the (previously reverted) D3908 patch.

The patch differs from D3908 in two things:

  • it only copies and installs machine/_types.h and machine/_stdint.h on platforms where it's necessary (FreeBSD 9.2, currently) and
  • it installs the two files mentioned in lib/clang/x.y.z/include/machine and not lib/clang/x.y.z/include and thus does not break resulting installation.

Diff Detail

Event Timeline

kutuzov.viktor.84 retitled this revision from to Add clang headers that fix machine-dependent definitions on FreeBSD 9.2 (2nd attempt).
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: rsmith, theraven.
kutuzov.viktor.84 added subscribers: kcc, samsonov, tobiasfar and 2 others.

Richard, answering your question:

I'm also not sure why this patch makes any sense: it seems you're trying to work around FreeBSD's broken /usr/include files? That doesn't seem like something we should be doing to me, but maybe I'm missing some background here?

Well, I'm not sure what you would propose in situation like that, but the point is FreeBSD 9.2 headers define some things incorrectly in 32-bit mode and that patch fix them so we can pass 32-bit sanitizer tests.

Richard, somehow your questions didn't pass to this page; I will quote them here.

OK, lots of code has bugs. In the vast majority of cases, we don't teach the compiler to work around those bugs. Why is this case different? Shouldn't we tell people to patch their system headers instead? If we ship replacement system headers in every case where we find a problem, we'll have a maintenance nightmare; clang should try very hard not to grow its own version of fixincludes.

Yes, I realize that LLVM developers generally do not like to fix/workaround third-party defects and try to keep things small and simple. Well, I don't mind we do not commit this patch and leave it to FreeBSD 9.2 users to alter their systems the ways they like in order to build LLVM/clang and enable sanitizers. However, doing so several points should be taken into account, I believe. Here's the (most likely incomplete) list:

  • It is not actually a bug, but rather incomplete 32-bit support. v9.3 adds more complete 32-bit support and (presumably) has no problems with headers in 32-bit mode, but there's no an "official patch" for v9.2, nor it is expected;
  • According to FreeBSD developers, v9.2 is a kind of LTS (long-term support) version and is still in wide use. The FreeBSD sanitizers buildbot is running v9.2;
  • Altering headers in omission of an "official patch" is a system-wide change that potentially break other software that workaround the issues with 32-bit definitions. Note that patching clang headers does not break anything as the HEAD version is known not to be the system compiler on any OS;
  • This work on porting sanitizers to v9.2 helps to fix and improve subsequent FreeBSD versions so they compile and run smoothly without too much effort from the user's side. OTOH, once v9.2 is out of date enough, the workaround can be eliminated form the clang code base without affecting current FreeBSD users.

Also, why does this affect the sanitizer tests? Are they including these headers somehow?

Yes, since the sanitizer tests build and run in both 64-bit and 32-bit modes.

Can we fix them by detecting and fixing the bad typedefs/macros in those tests?

Yes, technically. In practice, I suspect the sanitizers team would be pretty much against following this way as that would mean adding a lot of FreeBSD 9.2-specific code to most of the tests.

kutuzov.viktor.84 abandoned this revision.Sep 3 2014, 3:14 AM

Just for the record: the Richard Smith's response from the llvm-commits mailing list reads:

I do not think it is our job to "fix" partially-implemented OS features. This patch would take us in a very troubling direction, and it's not clear that we would ever be able to remove this hack. Please try to find another way to get the result you desire. If the sanitizers + FreeBSD 9.2 needs some patch to be applied to the system headers, then that patch should be provided by FreeBSD or possibly by the sanitizers, and *not* by the compiler.

So, 32-bit sanitizers tests are "officially" not supported on FreeBSD 9.2 and so we will proceed with the (upgraded) FreeBSD 10.0 sanitizers buildbot.

Thanks.

emaste added a comment.Sep 3 2014, 9:26 AM

So, 32-bit sanitizers tests are "officially" not supported on FreeBSD 9.2 and so we will proceed with the (upgraded) FreeBSD 10.0 sanitizers buildbot.

That is reasonable, as -m32 compilation is officially not supported by
FreeBSD 9.x anyway. I merged as much as I could in time for 9.3 on a
best-effort basis, although I'm not sure if I addressed this specific
case or not.