Types can end up in the wrong namespace #2556

glandium · 2023-06-15T08:14:13Z

This is a regression from 83f729f

Input C/C++ Header

I'm sorry this is not reduced as much as possible, but it's already significantly reduced. The problem does not happen using the output of --dump-preprocessed-input (it does require multiple separate headers, and even worse, it requires that some of them are included multiple times)

The complete set of files involved in this somehow reduced testcase is: https://drive.google.com/file/d/1vldgLGgcslyeUTlcHzMd4r0WVZNY2Wht/view?usp=sharing

Bindgen Invocation

Assuming the testcase.tar.gz archive is extracted to /tmp/testcase, run the cli with

cargo run --release -p bindgen-cli -- /tmp/testcase/mozilla/GeckoBindings.h --allowlist-type nsSimpleContentList --allowlist-type nsSize --enable-cxx-namespaces --no-derive-default --generate types --ignore-methods --rust-target 1.68 -- -x c++ -std=c++17 -fno-sized-deallocation -fno-aligned-new -DTRACING=1 -DIMPL_LIBXUL -DMOZILLA_INTERNAL_API -DRUST_BINDGEN -I/tmp/testcase -I/tmp/testcase/nspr -include /tmp/testcase/mozilla-config.h -include /tmp/testcase/nsStyleStruct.h -include /tmp/testcase/LayoutConstants.h

(using clang 16, I don't know for sure whether the command works with older versions, I know for sure it doesn't work with clang 11, because I have that in a corner)

Actual Results

The output contains the nsSize type in the root::mozilla namespace, although the type is defined in the root namespace.

There are many ways to make this disappear:

removing --allowlist-type nsSimpleContentList
in LayoutConstants.h, adding constexpr nsSize dummy(0, 0); above namespace mozilla (because yes, that's where the bogus namespace comes from)
not including nsStyleStruct.h from the command line
not including nsStyleStruct.h from mozilla/GeckoBindings.h
using the preprocessed input
creating a file that includes mozilla-config.h, nsStyleStruct.h, LayoutConstants.h and mozilla/GeckoBindings.h, and using that instead of passing those files on the command line.
etc.

The text was updated successfully, but these errors were encountered:

emilio · 2023-06-15T08:17:23Z

cc @reitermarkus @pvdrz

I commented on #2543 with some stuff that I found at a glance but I don't think it necessarily causes this bug...

emilio · 2023-06-15T08:23:28Z

@glandium I also needed -std=c++17 for the test-case to work fwiw, maybe you can update the OP?

emilio · 2023-06-15T08:34:01Z

I'm fairly confused at #2543. It only sorts the direct children of a cursor, how does it deal with C++ namespaces or other cursors with nested children for which we return ParseError::Recurse? Do those not need to be sorted?

emilio · 2023-06-15T08:49:48Z

Another probably more relevant data point. If I remove the -include flags and change them by actual #include directives in the first file, then it works. That probably explains why it doesn't happen in the preprocessed source.

emilio · 2023-06-15T08:50:52Z

We're probably relying on seeing a declaration before a template instantiation and somehow bindgen is getting the sorting wrong because of the -include flag.

glandium · 2023-06-15T08:54:57Z

Another probably more relevant data point. If I remove the -include flags and change them by actual #include directives in the first file, then it works. That probably explains why it doesn't happen in the preprocessed source.

See one of the many ways to make it work mentioned in OP ;)
That said, I think the reason it doesn't happen in the preprocessed source is different, because in that case, the locations are all associated with that one file.

glandium · 2023-06-15T08:57:25Z

BTW, I should mention that replacing the visit_sorted in parse with visit makes it work too (without touching parse_one or ClangSubItemParser)

emilio · 2023-06-15T10:24:24Z

Here's an actually reduced test-case. Still multiple headers because of the nature of the issue: reduced.tar.gz

To reproduce:

$ ./target/debug/bindgen --no-layout-tests reduced/GeckoBindings.h --enable-cxx-namespaces --generate types -- -x c++ -std=c++17 -include reduced/nsStyleStruct.h -include reduced/LayoutConstants.h

The nsSize struct ends up in the foo namespace, rather than in the root.

emilio · 2023-06-15T10:32:28Z

Further reduced: reduced.tar.gz

$ ./target/debug/bindgen --no-layout-tests reduced/GeckoBindings.h -- -x c++ -include reduced/nsStyleStruct.h -include reduced/LayoutConstants.h

Actual output:

/* automatically generated by rust-bindgen 0.66.0 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct foo_nsSize {
    pub width: ::std::os::raw::c_int,
    pub height: ::std::os::raw::c_int,
}
extern "C" {
    #[link_name = "\u{1}_ZN3fooL22kFallbackIntrinsicSizeE"]
    pub static foo_kFallbackIntrinsicSize: foo_nsSize;
}

Expected output:

/* automatically generated by rust-bindgen 0.66.0 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct nsSize {
    pub width: ::std::os::raw::c_int,
    pub height: ::std::os::raw::c_int,
}
extern "C" {
    #[link_name = "\u{1}_ZN3fooL22kFallbackIntrinsicSizeE"]
    pub static foo_kFallbackIntrinsicSize: nsSize;
}

emilio · 2023-06-15T11:17:37Z

Not a surprise of course, but the sorting is going rather wrong:

before sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize")),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo"))
]
after sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo")),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/nsStyleStruct.h:3:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize"))
]

I don't think the source order comparison is working right to begin with, it only deals with one level of includes?

In the case of LayoutConstants.h, for example, which is not included by anything, anything in it would be considered first than anything in nsSize.h, even though nsSize.h is included by LayoutConstants.h!

emilio · 2023-06-15T11:18:29Z

The other thing I don't understand is why do we need to do this sorting at all? The original sorting from clang looks right.

This disables (hopefully temporarily) source order sorting, for causing correctness regressions like rust-lang#2556. Fixes rust-lang#2556.

reitermarkus · 2023-06-15T11:51:42Z

The other thing I don't understand is why do we need to do this sorting at all?

clang groups all items by type. So e.g. all macros are sorted correctly relative to each other, but all macros are sorted before e.g. all variables.

Now if you have this

#define VAR 1
#undef VAR
const int VAR = 2;

it would generate VAR 1 with clang sorting. By sorting based on source order, we can reason that VAR was undefined since we see a constant with the same name later, and correctly generate VAR 2.

emilio · 2023-06-15T11:57:59Z

@reitermarkus why do we need the var to be sorted correctly relatively to the macro? The var is already correctly macro-expanded etc, that's the only reason bindgen can deal with token pasting etc. The macro information that clang provides should be treated probably rather separately to everything else. And that still would work for a test-case like the one you pasted, right?

In any case the source order sorting as implemented today is clearly incorrect. I ran out of time to fix it for the day, so I sent a PR to disable temporarily, see #2558.

I think that's the best course of action for now until it can be adapted to deal with multiple layers of includes properly.

reitermarkus · 2023-06-15T12:16:55Z

The macro information that clang provides should be treated probably rather separately to everything else.

In #2369, it is treated separately. The issue is, macros are parsed first, so in codegen macros are also generated first. And we generate constants for variable-like macros as well as for real constants.

emilio · 2023-06-15T13:24:50Z

Ok, seems those conflicts should be dealt with at codegen time? Or am I missing something? Or, rather, there's no reason we couldn't run codegen for macro-generated constants later, to "favor" real constants

reitermarkus · 2023-06-15T18:14:11Z

to "favor" real constants

Then this

const int VAR = 1;
#define VAR 2

would wrongly generate VAR 1. Granted this is very much an edge case.

pvdrz · 2023-06-15T20:11:22Z

Not a surprise of course, but the sorting is going rather wrong:

before sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize")),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo"))
]
after sorting: [
  Cursor(nsSize.h kind: inclusion directive, loc: ./reduced/LayoutConstants.h:1:1, usr: None),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/GeckoBindings.h:1:1, usr: None),
  Cursor(nsStyleStruct.h kind: inclusion directive, loc: reduced/GeckoBindings.h:2:1, usr: None),
  Cursor(foo kind: Namespace, loc: ./reduced/LayoutConstants.h:3:11, usr: Some("c:@N@foo")),
  Cursor(nsSize.h kind: inclusion directive, loc: reduced/nsStyleStruct.h:3:1, usr: None),
  Cursor(nsSize kind: StructDecl, loc: reduced/nsSize.h:3:8, usr: Some("c:@S@nsSize"))
]

I don't think the source order comparison is working right to begin with, it only deals with one level of includes?

In the case of LayoutConstants.h, for example, which is not included by anything, anything in it would be considered first than anything in nsSize.h, even though nsSize.h is included by LayoutConstants.h!

I noticed this in 83f729f but decided to ignore it for some reason 🤔. This is starting to sound like you need to topologically sort the cursors based on the partial order defined there instead of just sorting them in a binary way. In that way any cursor that appears before another in the list is guaranteed to not depend on the latter.

* clang: Clean up source order sorting. This doesn't change behavior but is easier to debug and reason about (because you have the relevant cursors there). * clang: Disable source order sorting for now. This disables (hopefully temporarily) source order sorting, for causing correctness regressions like #2556. Fixes #2556. * tests: Add a test for #2556 * Remove merge artifact * Update clang.rs --------- Co-authored-by: Christian Poveda Ruiz <31802960+pvdrz@users.noreply.github.com>

glandium · 2023-06-15T21:24:49Z

Can you spin a 0.66.1 release with the fix?

glandium · 2023-06-15T21:31:33Z

(there are failing tests on CI, though)

pvdrz · 2023-06-15T21:42:23Z

Ugh I think that's because the clang 5 and 9 tests needed to be reverted too. I'll take a look.

The PR for 0.66.1 has already been open #2559

emilio added a commit to emilio/rust-bindgen that referenced this issue Jun 15, 2023

clang: Disable source order sorting for now.

2b36168

This disables (hopefully temporarily) source order sorting, for causing correctness regressions like rust-lang#2556. Fixes rust-lang#2556.

emilio added a commit to emilio/rust-bindgen that referenced this issue Jun 15, 2023

tests: Add a test for rust-lang#2556

eebb3eb

emilio mentioned this issue Jun 15, 2023

clang: Disable source order sorting for now. #2558

Merged

emilio added a commit to emilio/rust-bindgen that referenced this issue Jun 15, 2023

tests: Add a test for rust-lang#2556

9a6a078

pvdrz closed this as completed in c14e53e Jun 15, 2023

pvdrz mentioned this issue Jun 15, 2023

Fix expectation tests for clang 5 #2560

Merged

reitermarkus mentioned this issue Jun 16, 2023

Fix source order sorting. #2561

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Types can end up in the wrong namespace #2556

Types can end up in the wrong namespace #2556

glandium commented Jun 15, 2023 •

edited

Loading

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

glandium commented Jun 15, 2023

glandium commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

pvdrz commented Jun 15, 2023

glandium commented Jun 15, 2023

glandium commented Jun 15, 2023

pvdrz commented Jun 15, 2023

Types can end up in the wrong namespace #2556

Types can end up in the wrong namespace #2556

Comments

glandium commented Jun 15, 2023 • edited Loading

Input C/C++ Header

Bindgen Invocation

Actual Results

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

glandium commented Jun 15, 2023

glandium commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

emilio commented Jun 15, 2023

reitermarkus commented Jun 15, 2023

pvdrz commented Jun 15, 2023

glandium commented Jun 15, 2023

glandium commented Jun 15, 2023

pvdrz commented Jun 15, 2023

glandium commented Jun 15, 2023 •

edited

Loading