Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build fails on 32bit architectures #6609

Closed
ignatenkobrain opened this issue Feb 15, 2020 · 16 comments
Closed

Build fails on 32bit architectures #6609

ignatenkobrain opened this issue Feb 15, 2020 · 16 comments
Labels
bug
Milestone

Comments

@ignatenkobrain
Copy link

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

../src/wutil.cpp: In function 'int fd_check_is_remote(int)':
../src/wutil.cpp:311:14: error: narrowing conversion of '4283649346' from 'unsigned int' to 'int' [-Wnarrowing]
  311 |         case 0xFF534D42:  // CIFS_MAGIC_NUMBER
      |              ^~~~~~~~~~
@faho
Copy link
Member

@faho faho commented Feb 15, 2020

I'd assume this works if we just add a "u" to the end of the literal? I.e.

        case 0xFF534D42u:  // CIFS_MAGIC_NUMBER

@faho faho added the bug label Feb 15, 2020
@faho faho added this to the fish 3.1.1 milestone Feb 15, 2020
@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

Let me quickly test it.

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

@faho, no that does not help:

../src/wutil.cpp: In function 'int fd_check_is_remote(int)':
../src/wutil.cpp:311:14: error: narrowing conversion of '4283649346' from 'unsigned int' to 'int' [-Wnarrowing]
  311 |         case 0xFF534D42u: // CIFS_MAGIC_NUMBER
      |              ^~~~~~~~~~~

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

I guess we'd need to use statfs64()?

But I'm not very sure if that is not just a bug somewhere in glibc.

       The  original  Linux  statfs()  and  fstatfs() system calls were not designed with extremely large file sizes in mind.  Subsequently, Linux 2.6 added new statfs64() and
       fstatfs64() system calls that employ a new structure, statfs64.  The new structure contains the same fields as the original statfs structure, but the sizes  of  various
       fields are increased, to accommodate large file sizes.  The glibc statfs() and fstatfs() wrapper functions transparently deal with the kernel differences.

@fweimer, do you think this is some regression in new glibc or it is supposed to be this way?

@faho
Copy link
Member

@faho faho commented Feb 15, 2020

Maybe try adding the "u" to all cases in that switch?

The man page for fstatfs (linux man-pages 5.05) says:

The __fsword_t type used for various fields in the statfs structure definition is a glibc internal type, not intended for public use. This leaves the programmer in a bit of a conundrum when trying to copy or compare these fields to local variables in a program. Using unsigned int for such variables suffices on most systems.

And here it's still complaining that we're converting from unsigned int to int, so to me it seems like we're missing a cause for conversion.

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

I've tried both adding u suffix to all cases and switching to statfs64, none of which help ;(

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

diff --git a/src/wutil.cpp b/src/wutil.cpp
index 4971e0d63..aaa12a5ee 100644
--- a/src/wutil.cpp
+++ b/src/wutil.cpp
@@ -264,7 +264,7 @@ int fd_check_is_remote(int fd) {
     }
     // Linux has constants for these like NFS_SUPER_MAGIC, SMB_SUPER_MAGIC, CIFS_MAGIC_NUMBER but
     // these are in varying headers. Simply hard code them.
-    switch (buf.f_type) {
+    switch ((unsigned int)buf.f_type) {
         case 0x6969:      // NFS_SUPER_MAGIC
         case 0x517B:      // SMB_SUPER_MAGIC
         case 0xFF534D42:  // CIFS_MAGIC_NUMBER

But this patch fixes it.

@faho faho closed this as completed in 399a716 Feb 15, 2020
@faho
Copy link
Member

@faho faho commented Feb 15, 2020

Applied with comments (and the "u") to add some clarity.

Thanks!

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 15, 2020

What platform is this on? We're building 32-bit binaries and shipping them without problems on a number of Linux distributions.

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

@zanchey Fedora Rawhide. So this might be related to the new glibc/gcc

@fweimer
Copy link

@fweimer fweimer commented Feb 15, 2020

The warning for case 0xFF534D42u looks like a compiler bug.

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

FTR I've opened bug against gcc in RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1803333

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 15, 2020

The manpage for statfs says:

The __fsword_t type used for various fields in the statfs structure definition is a glibc internal type, not intended for public use. This leaves the programmer in a bit of a conundrum when trying to copy or compare these fields to local variables in a program. Using unsigned int for such variables suffices on most systems.

So I wonder if there's something upstream that's changed here.

@ignatenkobrain
Copy link
Author

@ignatenkobrain ignatenkobrain commented Feb 15, 2020

Not a compiler bug. The C++ standard requires this to be diagnosed.

That value cannot be converted to an int without changing its value. If you want the int value corresponding to that bit pattern, you need to use (int)0xFF534D42. If the switch is meant to be using unsigned, cast the switch expression to unsigned int (as suggested in the github issue).

Adding a u suffix makes no difference because the type is already unsigned int, because the value 0xFF534D42 is too large for int. The problem is that the switch expression is an int, so any case which can't be represented as an int is ill-formed.

@faho
Copy link
Member

@faho faho commented Feb 15, 2020

The problem is that the switch expression is an int, so any case which can't be represented as an int is ill-formed.

This is the part I don't understand? The docs say it's usable as an unsigned int (disregarding this weird bit about it really being a glibc-internal type), so why is it used as an int?

Not that it matters, we have an acceptable workaround.

@zanchey
Copy link
Member

@zanchey zanchey commented Feb 15, 2020

I spent half an hour trying to work out whether this was right, but am I right in saying:

  • in libc, f_type is __fsword_t which is a signed int on 32-bit platforms and a signed long on 64-bit platforms
  • in the kernel, f_type is __statfs_word which is a signed long on 64-bit and unsigned 32-bit int on 32-bit platforms
  • The compiler takes 0xFF534D42 as a literal which is too big to fit in an int, so gets stored as an unsigned int by the compiler
  • This fits into the type defined in the kernel, but not in libc (on 32-bit platforms)

Now I start to go off into the weeds...

  • So the representation of 0xFF534D42 gets passed from the kernel (where it is equivalent to decimal 4283649346) to libc (where it is equivalent to decimal something negative, I can't work out two's complement, I think this is platform dependent anyway?), and there's limited type-checking of the kernel-libc interface (this is conjecture on my part, I don't actually know)

I guess what I'm wondering is whether this code is actually right - is the value from the kernel actually going to match the case?

And this is where things get really weird:

#include <stdio.h>
#include <sys/vfs.h>

int main() {
    struct statfs b {};
    if(statfs("/mnt", &b) < 0) {
        perror("statfs");
        return 1;
    }
    printf("f_type %x\n", b.f_type);
    if (b.f_type == 0xFF534D42) {
        return 0;
    } else {
        return 1;
    }
}

Run against a CIFS mountpoint, I get...

> g++ check-fstype.cpp ; and ./a.out
f_type fe534d42
[1]>

???

zanchey pushed a commit that referenced this issue Feb 22, 2020
This was a weird case of the integer converseys.

Fixes #6609.

(cherry picked from commit 399a716)
zanchey added a commit that referenced this issue Apr 29, 2020
The manual page for statfs(2) only lists SMB_SUPER_MAGIC and
CIFS_MAGIC_NUMBER, but it turns out there's a third type of CIFS/SMB
mount, represented by SMB2_MAGIC_NUMBER.

Haunting me from #6609.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

No branches or pull requests

4 participants