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

const field inside class is not handled correctly on Windows #271

Closed
upsuper opened this issue Nov 16, 2016 · 11 comments
Closed

const field inside class is not handled correctly on Windows #271

upsuper opened this issue Nov 16, 2016 · 11 comments

Comments

@upsuper
Copy link
Contributor

upsuper commented Nov 16, 2016

Test code:

class A {
  const size_t count;
};

On Windows, it generates:

#[repr(C)]
#[derive(Debug, Copy)]
pub struct A {
    pub count: A_size_t,
}

where A_size_t is never defined.

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

I don't think this would reproduce if size_t was defined, does it?

i.e., I can't reproduce if I instead:

typedef unsigned size_t; // Or define it in some other way.

class A {
  const size_t count;
};

@upsuper
Copy link
Contributor Author

upsuper commented Nov 16, 2016

It doesn't matter.

typedef unsigned long long size_t;
class A {
  const size_t count;
};

still gives me

#[repr(C)]
#[derive(Debug, Copy)]
pub struct A {
    pub count: A_size_t,
}

FWIW, I can only reproduce this on Windows. It works fine on my macOS. I have no idea why...

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

Gah, github-debugging time again :).

Can you use --emit-clang-ast and paste it here?

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

The bindgen logs will also help, I guess.

@upsuper
Copy link
Contributor Author

upsuper commented Nov 16, 2016

This is the log with RUST_LOG=bindgen cargo run -- test.h --emit-clang-ast -- -x c++ -std=c++14, and test.h is the file I pasted in my previous comment.

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

Can you use RUST_LOG=libbindgen (the crate was split in #204)? Also, it seems clang is ignoring the -x c++ argument, per the first warning, does it reproduce if instead of test.h it you call the file test.hpp?

@upsuper
Copy link
Contributor Author

upsuper commented Nov 16, 2016

This is log.txt from RUST_LOG=bindgen,libbindgen cargo run -- test.hpp --emit-clang-ast -- -std=c++14.

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

So it's again an MSVC backend bug, where they report no USR for the typedef declaration, so we don't find it and assume that "const size_t" is a type declared inside of A...

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

@upsuper, does the following patch make it work for you?

diff --git a/libbindgen/src/ir/context.rs b/libbindgen/src/ir/context.rs
index a1ec8c7..bda3e3c 100644
--- a/libbindgen/src/ir/context.rs
+++ b/libbindgen/src/ir/context.rs
@@ -222,7 +222,7 @@ impl<'ctx> BindgenContext<'ctx> {
                 error!("Valid declaration with no USR: {:?}, {:?}",
                        declaration,
                        location);
-                return;
+                TypeKey::Declaration(declaration)
             };

             let old = self.types.insert(key, id);

@upsuper
Copy link
Contributor Author

upsuper commented Nov 16, 2016

Yes, it seems to work. Thanks.

@emilio
Copy link
Contributor

emilio commented Nov 16, 2016

I added the fix in #260, should be merged soon.

Thank you for reporting it :)

bors-servo pushed a commit to servo/servo that referenced this issue Dec 10, 2016
Use build script to generate binding files

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14244)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Dec 10, 2016
Use build script to generate binding files

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14244)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Dec 10, 2016
Use build script to generate binding files

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14244)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Dec 10, 2016
Use build script to generate binding files

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14244)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Dec 10, 2016
Use build script to generate binding files

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14244)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 4, 2017
…m upsuper:buildtime-bindgen); r=emilio

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

@Manishearth @emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 872ec89a9c546eb05246b5047aabfc032d140eff
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…m upsuper:buildtime-bindgen); r=emilio

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

Manishearth emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 872ec89a9c546eb05246b5047aabfc032d140eff

UltraBlame original commit: 78635ebabfc9e63971829a549388d1b61a75a503
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…m upsuper:buildtime-bindgen); r=emilio

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

Manishearth emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 872ec89a9c546eb05246b5047aabfc032d140eff

UltraBlame original commit: 78635ebabfc9e63971829a549388d1b61a75a503
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…m upsuper:buildtime-bindgen); r=emilio

<!-- Please describe your changes on the following line: -->
This is a WIP patch to make rust-bindgen a build time dependency of style component for stylo. This should make things more automatic. I convert majority of `regen.py` to `build_gecko.rs`.

It currently doesn't work on Windows, because of rust-lang/rust-bindgen#271 (it works when I fix the generated file manually, though). I haven't tested other platforms.

It would break servo's CI for geckolib, because:
1. it needs libclang (which is probably easy to solve)
2. it needs `MOZ_OBJDIR` to be passed in so that it can generate bindings

Manishearth emilio Do you agree with this change? Do you have suggestion for the issues above?

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 872ec89a9c546eb05246b5047aabfc032d140eff

UltraBlame original commit: 78635ebabfc9e63971829a549388d1b61a75a503
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants