Fix build failure in Alpha architecture

[henrich]

Taken from Debian package, see https://bugs.debian.org/962676
If it would be merged, we Debian can reduce distro-specific patches.
Please consider applying, thanks.

[facebook-github-bot]

Hi @henrich!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[Cyan4973]

So I guess it means __alpha__ platform never claims POSIX 2008 compliance, since it probably is not,
yet nonetheless support struct timespec and st_mtime as extended features ?

The surprising part here is that, in this construction, __alpha__ is presumed enough of a hint to guarantee the support of struct timespec, no matter the software level. I would have expected __alpha__ to be much older than struct timespec, and therefore logically, older variants of software for __alpha__ would not provide struct timespec. Though I don't know enough about __alpha__ history to be sure about that.

Another avenue could have been to declare PLATFORM_POSIX_VERSION = 200809L when the right set of conditions is present (__alpha__ and maybe another condition), but this would imply support of other stuff beyond struct timespec.

[Cyan4973]

What about :

#if (PLATFORM_POSIX_VERSION >= 200809L || defined(__alpha__)) && defined(st_mtime) 

[felixhandte]

@Cyan4973, as the linked bug states, Alpha does not define st_mtime (which makes it non-compliant with POSIX...). So that won't work.

[Cyan4973]

okay,
so st_mtime is in fact not required,
it's just employed as a way to improve odds that struct timespec.st_mtim is actually supported ?

[felixhandte]

Correct. We previously found some platform (I forget which) which claims to be POSIX 2008 but doesn't have struct timespec and we detect that by observing that they don't define st_mtime.

[felixhandte]

I'm uncomfortable with this fix as proposed. While this is a reasonable patch for Debian to ship, since they can make guarantees about the environment this code is being shipped in, I don't think we can do the same. So I agree with @Cyan4973--I can only assume there are old or maybe even current OS runtimes for Alpha that define __alpha__ but which do not have st_mtime.

We need a good signal to detect this capability. It sounds like this might be difficult, since it's apparent the environment is non-trivially non-compliant with POSIX. (For what it's worth, the linked bug is incorrect: POSIX 2008 requires that st_mtime is a macro: see page 370 line 12824 of this pdf. And POSIX 2001 requires struct utimbuf: page 1563 line 48237 of this pdf.)

In the absence of anything better, we could just disable the utime() path for Alpha, which will cause UTIL_utime() to just fall back to being a no-op.

[terrelln]

nit: Can you add parenthesis to clarify the statement?

Suggested change

	#if (PLATFORM_POSIX_VERSION >= 200809L) && defined(st_mtime) || defined(__alpha__)
	#if ((PLATFORM_POSIX_VERSION >= 200809L) && defined(st_mtime)) || defined(__alpha__)

[arnout]

FYI, this is actually a bug in glibc itself. I'm working on a fix upstream and will report back.

[arnout]

FYI, this is actually a bug in glibc itself. I'm working on a fix upstream and will report back.

Sorry, I was wrong - there is a glibc bug on microblaze, but on alpha the issue is really in zstd itself. Sorry for the noise.

[felixhandte]

@henrich, @arnout, we're ramping up to our next release and I'd like to get a fix for this issue merged.

I can make the test as follows:

#if ((PLATFORM_POSIX_VERSION >= 200809L) && (defined(st_mtime) || defined(__alpha__)))
  struct timespec timebuf;
  // ...
#elif (PLATFORM_POSIX_VERSION >= 200112L)
  struct utimbuf timebuf;
  // ...
#else
  // do nothing
#endif

Is it correct to assume all Alpha systems with _POSIX_VERSION >= 200809L expose a struct timespec? Otherwise this exemption is dangerous.

[felixhandte]

In the absence of any activity, I'm going to close this.

If anyone with an alpha system can comment on the appropriateness of the proposed fix, I'm happy to re-open this and get a fix merged.