Open Bug 1795899 Opened 2 years ago Updated 9 months ago

build errors after updating Clang 11 to 14 - expected `u64`, found `usize`

Categories

(Firefox Build System :: Toolchains, defect)

Product:
Firefox Build System
Component:
Toolchains
Version:
Firefox 102
Type:
defect

Tracking

(Not tracked)

Status:
UNCONFIRMED

People

(Reporter: petr.sumbera, Unassigned)

Details

Attachments

(3 files)

output of ./target/debug/bindgen --size_t-is-usize t.h
1.46 KB, text/plain
Details
bindings.rs.diff
19.89 KB, patch
Details | Diff | Splinter Review
__bindgen.ii from failed build
5.35 MB, text/plain
Details

Steps to reproduce:

My Solaris build (Firefox 102.3.0) fails when I try to change Clang version from 11 to 14:

error[E0308]: mismatched types
  --> tools/profiler/rust-api/src/gecko_bindings/glue.rs:48:13
   |
48 |             marker_name.len(),
   |             ^^^^^^^^^^^^^^^^^ expected `u64`, found `usize`
   |
help: you can convert a `usize` to a `u64` and panic if the converted value doesn't fit
   |
48 |             marker_name.len().try_into().unwrap(),
   |                              ++++++++++++++++++++

error[E0308]: mismatched types
  --> tools/profiler/rust-api/src/json_writer.rs:29:17
   |
29 |                 name.len(),
   |                 ^^^^^^^^^^ expected `u64`, found `usize`
   |
help: you can convert a `usize` to a `u64` and panic if the converted value doesn't fit
   |
29 |                 name.len().try_into().unwrap(),
   |                           ++++++++++++++++++++

error[E0308]: mismatched types
  --> tools/profiler/rust-api/src/json_writer.rs:42:17
   |
42 |                 name.len(),
   |                 ^^^^^^^^^^ expected `u64`, found `usize`
   |
help: you can convert a `usize` to a `u64` and panic if the converted value doesn't fit
   |
42 |                 name.len().try_into().unwrap(),
   |                           ++++++++++++++++++++

Emilio, any idea about this? Not sure how to debug it. I just think it might have something to do with cbindgen.

Flags: needinfo?(emilio)

The Bugbug bot thinks this bug should belong to the 'Core::Gecko Profiler' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Component: Untriaged → Gecko Profiler
Product: Firefox → Core

So... this is about bindgen. If you do:

$ git clone https://github.com/rust-lang/rust-bindgen
$ cd rust-bindgen
$ cargo build
$ ./target/debug/bindgen --size-t-is-usize t.h

Where t.h contains something like:

#include <stddef.h>

static const size_t MY_SIZE = 10;

what does it generate?

This line should make sure that it generates usize rather than u64: https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/tools/profiler/rust-api/build.rs#65

Flags: needinfo?(emilio) → needinfo?(petr.sumbera)
Component: Gecko Profiler → Toolchains
Product: Core → Firefox Build System

Please see attached file. But I don't see there any difference to system with old Clang 11.

Flags: needinfo?(petr.sumbera)

Yeah, that's the expected output. Does a mozilla-central build show the same error? Or does this happen only on ESR?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

Yeah, that's the expected output. Does a mozilla-central build show the same error? Or does this happen only on ESR?

I can try it. But it will take some time to setup it...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

This line should make sure that it generates usize rather than u64: https://searchfox.org/mozilla-central/rev/0a2eba79c24300ce0539f91c1bebac2e75264e58/tools/profiler/rust-api/build.rs#65

Also tried to change the line to .size_t_is_usize(false);. But don't see any difference...

I can also confirm that latest mozilla-central sources fail the same way as ESR.

Attached patch bindings.rs.diff — — Splinter Review

I have compared x86_64-pc-solaris/release/build/gecko-profiler-5b5e705920deb946/out/gecko/bindings.rs as they are on build system with Clang 11 and 14. They are different...

I have tried to find the first "bad" Clang commit and it is:

https://github.com/llvm/llvm-project/commit/af27466c50398e3f04372850370eab8dc8abbb92

Reland "[AST] Add UsingType: a sugar type for types found via UsingDecl"

This reverts commit cc56c66.
Fixed a bad assertion, the target of a UsingShadowDecl must not have
local qualifiers, but it can be a typedef whose underlying type is qualified.

(In reply to Petr Sumbera from comment #9)

Created attachment 9299241 [details] [diff] [review]
bindings.rs.diff

I have compared x86_64-pc-solaris/release/build/gecko-profiler-5b5e705920deb946/out/gecko/bindings.rs as they are on build system with Clang 11 and 14. They are different...

What is the size_t definition of solaris? This code should be turning size_t into usize... https://searchfox.org/mozilla-central/rev/12a18f7e112a4dcf88d8441d439b84144bfbe9a3/third_party/rust/bindgen/src/codegen/mod.rs#4608

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

What is the size_t definition of solaris?

size_t is defined as unsigned long and it has 8 bytes on 64bits system/program.

How is std::size_t defined in the C++ stdlib?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

How is std::size_t defined in the C++ stdlib?

$ echo | g++ -dM -E -x c++ - | grep __SIZE_TYPE__
#define __SIZE_TYPE__ long unsigned int

where in /usr/gcc/12/include/c++/12.1.0/x86_64-pc-solaris2.11/bits/c++config.h I see

namespace std
{
  typedef __SIZE_TYPE__         size_t;

I have added following debug print:

--- firefox-102.4.0/third_party/rust/bindgen/src/codegen/mod.rs
+++ firefox-102.4.0/third_party/rust/bindgen/src/codegen/mod.rs
@@ -4413,6 +4413,7 @@
     ) -> Option<proc_macro2::TokenStream> {
         // FIXME: We could use the inner item to check this is really a
         // primitive type but, who the heck overrides these anyway?
+println!("XXX {}", name);
         Some(match name {
             "int8_t" => primitive_ty(ctx, "i8"),
             "uint8_t" => primitive_ty(ctx, "u8"),

and compared output of these between ok and bad build:

--- list-ok-gecko-profiler-only
+++ list-bad
@@ -39,7 +39,6 @@
 [gecko-profiler 0.1.0] XXX pointer
 [gecko-profiler 0.1.0] XXX element_type
 [gecko-profiler 0.1.0] XXX index_type
-[gecko-profiler 0.1.0] XXX size_t
 [gecko-profiler 0.1.0] XXX pointer
 [gecko-profiler 0.1.0] XXX element_type
 [gecko-profiler 0.1.0] XXX reference
@@ -93,9 +92,7 @@
 [gecko-profiler 0.1.0] XXX KeyType
 [gecko-profiler 0.1.0] XXX ValueType
 [gecko-profiler 0.1.0] XXX Impl
-[gecko-profiler 0.1.0] XXX size_t
 [gecko-profiler 0.1.0] XXX ElementType
-[gecko-profiler 0.1.0] XXX size_t
 [gecko-profiler 0.1.0] XXX int64_t
 [gecko-profiler 0.1.0] XXX uint8_t
 [gecko-profiler 0.1.0] XXX uint32_t
@@ -264,31 +261,6 @@
..

Seems that only fraction of size_t gets to debug print:

$ grep size_t$ list-bad | wc -l
4
$ grep size_t$ list-ok-gecko-profiler-only | wc -l
32

Further I see that:
https://searchfox.org/mozilla-central/rev/12a18f7e112a4dcf88d8441d439b84144bfbe9a3/third_party/rust/bindgen/src/codegen/mod.rs#3543
gets Int(ULong) instead of Alias(TypeId(ItemId(20236))).

I wonder whether this issue is really Solaris specific? Do you already use libclang 14?

Flags: needinfo?(emilio)

Yeah, we build with clang 14 already:

$ ~/.mozbuild/clang/bin/clang --version                                                                                                                                                                                                    
clang version 14.0.5 (taskcluster-DYLBH6aFRYKs1iBHcPaFcw)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /home/emilio/.mozbuild/clang/bin

I wonder if there's a mismatch on your build environment between libclang and clang or something? But still shouldn't cause this

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

I wonder if there's a mismatch on your build environment between libclang and clang or something? But still shouldn't cause this

I don't use clang. I do use GCC for Firefox build. Have you tries GCC with libclang 14?

Adding some more debug prints I see that inner_item here differs:
https://searchfox.org/mozilla-esr102/rev/4b7af3a134800221659df4a05bf29dd55c23ceea/third_party/rust/bindgen/src/codegen/mod.rs#770

prior the Clang commit Reland "[AST] Add UsingType: a sugar type for types found via UsingDecl" (see above comment 10):

Item { id: ItemId(20234), local_id: LazyCell { inner: UnsafeCell { .. } }, next_child_local_id: Cell { value: 1 }, canonical_name: LazyCell { inner: UnsafeCell { .. } }, path_for_whitelisting: LazyCell { inner: UnsafeCell { .. } }, comment: None, annotations: Annotations { opaque: false, hide: false, use_instead_of: None, disallow_copy: false, disallow_debug: false, disallow_default: false, private_fields: None, accessor_kind: None, constify_enum_variant: false, derives: [] }, parent_id: ItemId(20233), kind: Type(Type { name: Some("size_t"), layout: Some(Layout { size: 8, align: 8, packed: false }), kind: Alias(TypeId(ItemId(20236))), is_const: false }) }

and with the commit:

Item { id: ItemId(106935), local_id: LazyCell { inner: UnsafeCell { .. } }, next_child_local_id: Cell { value: 1 }, canonical_name: LazyCell { inner: UnsafeCell { .. } }, path_for_whitelisting: LazyCell { inner: UnsafeCell { .. } }, comment: None, annotations: Annotations { opaque: false, hide: false, use_instead_of: None, disallow_copy: false, disallow_debug: false, disallow_default: false, private_fields: None, accessor_kind: None, constify_enum_variant: false, derives: [] }, parent_id: ItemId(0), kind: Type(Type { name: Some("unsigned long"), layout: Some(Layout { size: 8, align: 8, packed: false }), kind: Int(ULong), is_const: false }) }

Note Type(Type { name: Some("size_t").. vs name: Some("unsigned long")..

Any other suggestion how to debug it further? Probably outside of Firefox directly with clang binaray?!

Emilio, do you have any suggestion how to reproduce it outside of Firefox? I mean using directly cbindgen like we have tried in comment #3? But at that time the output didn't differ...

Flags: needinfo?(emilio)

Can you apply this patch:

diff --git a/tools/profiler/rust-api/build.rs b/tools/profiler/rust-api/build.rs
index 2dd70ed55c368..a0aab8d5b734a 100644
--- a/tools/profiler/rust-api/build.rs
+++ b/tools/profiler/rust-api/build.rs
@@ -76,7 +76,7 @@ fn generate_bindings() {
         builder = builder.clang_arg(item);
     }
 
-    let bindings = builder
+    builder = builder
         .header(add_include("GeckoProfiler.h"))
         .header(add_include("ProfilerBindings.h"))
         .allowlist_function("gecko_profiler_.*")
@@ -97,7 +97,9 @@ fn generate_bindings() {
         .raw_line("pub use self::root::*;")
         // Tell cargo to invalidate the built crate whenever any of the
         // included header files changed.
-        .parse_callbacks(Box::new(CargoCallbacks))
+        .parse_callbacks(Box::new(CargoCallbacks));
+    builder.dump_preprocessed_input().unwrap();
+    let bindings = builder
         // Finish the builder and generate the bindings.
         .generate()
         // Unwrap the Result and panic on failure.

And attach here the .ii file generated by bindgen? Should be in tools/profiler/rust-api/__bindgen.ii after running. You should be able to run the cli tool on that with --size_t-is-usize and see the same broken output.

Flags: needinfo?(emilio) → needinfo?(petr.sumbera)
Flags: needinfo?(petr.sumbera)
Attachment #9301710 - Attachment mime type: application/octet-stream → text/plain

This is probably expected. Generated __bindgen.ii is the same with "bad" and "good" libclang.

But bindgen command ends with error:

 ./target/debug/bindgen --size_t-is-usize __bindgen.ii
warning: argument unused during compilation: '-isystem /usr/lib/amd64/clang/14.0.0/include' [-Wunused-command-line-argument]
warning: argument unused during compilation: '-isystem /usr/include' [-Wunused-command-line-argument]
__bindgen.ii:20138:40: error: C++ requires a type specifier for all declarations
__bindgen.ii:23052:72: error: redefinition of '_Mem_fn_traits<_Res (_Class::*)(_ArgTypes...) noexcept>'
__bindgen.ii:23047:72: note: previous definition is here
__bindgen.ii:23052:283: error: redefinition of '_Mem_fn_traits<_Res (_Class::*)(_ArgTypes..., ...) noexcept>'
__bindgen.ii:23047:275: note: previous definition is here
__bindgen.ii:23052:497: error: redefinition of '_Mem_fn_traits<_Res (_Class::*)(_ArgTypes...) const noexcept>'
__bindgen.ii:23047:481: note: previous definition is here
__bindgen.ii:23052:720: error: redefinition of '_Mem_fn_traits<_Res (_Class::*)(_ArgTypes..., ...) const noexcept>'
__bindgen.ii:23047:696: note: previous definition is here
__bindgen.ii:23052:946: error: redefinition of '_Mem_fn_traits<_Res (_Class::*)(_ArgTypes...) volatile noexcept>'

Any idea?

Do you have any comment to above please? I don't really know how to continue with debuging.

Flags: needinfo?(emilio)

Does bug 1801029 help? Without a machine to reproduce this on this is kind of a guess game for me right now... I'd be happy to debug this on a machine that repros this tho... Feel free to mail me and we can maybe video-call or ssh onto a machine or something and try to get this fixed?

Flags: needinfo?(emilio) → needinfo?(petr.sumbera)

Unfortunatelly Bug 1801029 didn't help.

Flags: needinfo?(petr.sumbera)

Petr was kind enough to let me ssh into a machine that reproduced the issue.

The following file, named t.hpp:

#include <string> // remove this include, then it works
#include <cstddef>
#include <stdint.h>

namespace mozilla {
class MarkerSchema;
}

extern "C" {
void gecko_profiler_marker_schema_set_chart_label(
    mozilla::MarkerSchema* schema, const char* label, size_t len);
}

reproduces the issue. out.rs, generated with:

$ LIBCLANG_PATH=/usr/lib/amd64 ./target/debug/bindgen --size_t-is-usize t.hpp  > out.rs

Contains:

extern "C" {
    pub fn gecko_profiler_marker_schema_set_chart_label(
        schema: *mut mozilla_MarkerSchema,
        label: *const ::std::os::raw::c_char,
        len: ::std::os::raw::c_ulong,
    );
}

While if I remove the <string> include, I get the expected:

extern "C" {
    pub fn gecko_profiler_marker_schema_set_chart_label(
        schema: *mut mozilla_MarkerSchema,
        label: *const ::std::os::raw::c_char,
        len: usize,
    );
}

So it seems there are multiple cstddefs on the system (one from clang, one from gcc), and which one goes first makes it work or not, which is why we didn't find this minimal STR before. Minimal reproducible example (WORKS use clang's stddef, the other one is gcc's):

// #define WORKS

#ifdef WORKS
typedef unsigned long size_t;
#else
namespace std {
typedef unsigned long size_t;
}
using std::size_t;
#endif

namespace mozilla {
class MarkerSchema;
}

extern "C" {
void gecko_profiler_marker_schema_set_chart_label(
    mozilla::MarkerSchema* schema, const char* label, size_t len);
}

So the difference is that in the "bad" case, the size_t in size_t len is Unexposed, rather than Typedef... So bindgen tries to go across the unexposed by looking at the canonical type...

Sent https://reviews.llvm.org/D140075 and https://github.com/rust-lang/rust-bindgen/pull/2372. Unfortunately both are needed to fix the bug but you can either apply those patches to clang (hard), or maybe make sure that you only use clang's stddef.h?

Not sure how easy to do that is, there seems to be a weird mixup going on?

Flags: needinfo?(petr.sumbera)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #32)

Sent https://reviews.llvm.org/D140075 and https://github.com/rust-lang/rust-bindgen/pull/2372. Unfortunately both are needed to fix the bug but you can either apply those patches to clang (hard),

Will you please find some time to get these pull request to upstream?

or maybe make sure that you only use clang's stddef.h?

Not sure how easy to do that is, there seems to be a weird mixup going on?

I don't really know how to do that. Unfortuatelly I'm missing big picture. I just see that Firefox build fails. I would need to limit it to some process exuction..

Flags: needinfo?(petr.sumbera)

Serg, do you know how can I get some eyes on the llvm patch above?

Flags: needinfo?(sguelton)

I've merged D140074 and added a comment on D140074:, let's continue the discussion to LLVM's phabricator.

Flags: needinfo?(sguelton)
Severity: -- → S3

Sorry to bug you about this. Not sure what is current status. Can you please find some time to look at it?

Flags: needinfo?(emilio)

Yeah so I need to look at my llvm proposed patch, and carry it to completion, but unfortunately I don't think I have the cycles to do it right now...

Flags: needinfo?(emilio)

Just for record.

I played with Solaris system headers and can build Firefox when I comment out following using std::size_t:

/usr/include/stddef.h:using std::size_t;
/usr/include/stdio.h:using std::size_t;
/usr/include/stdlib.h:using std::size_t;
/usr/include/string.h:using std::size_t;
/usr/include/time.h:using std::size_t;
/usr/include/uchar.h:using std::size_t;
/usr/include/wchar.h:using std::size_t;

But this is not good solution.

I also tried your patch to Clang (17.0.2) and Rust bindgen (as is in Firefox 115 ESR) and it works. I was able to build it. I applied only relevant code not tests. I think this is the right workaround for me now.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: