Fix native main() signature on 64bit by dkl · Pull Request #44906 · rust-lang/rust

dkl · 2017-09-28T20:16:16Z

Hello,

in LLVM-IR produced by rustc on x86_64-linux-gnu, the native main() function had incorrect types for the function result and argc parameter: i64, while it should be i32 (really c_int). See also #20064, #29633.

So I've attempted a fix here. I tested it by checking the LLVM IR produced with --target x86_64-unknown-linux-gnu and i686-unknown-linux-gnu. Also I tried running the tests (./x.py test), however I'm getting two failures with and without the patch, which I'm guessing is unrelated.

Add c_int for use in the compiler, assuming i32 for all targets as in libc.

To match the C signature, main() should be generated with C int type for the argc parameter and result, i.e. i32 instead of i64 on 64bit. That way it no longer relies on the upper 32 bits being zero, which I'm not sure is guaranteed by ABIs or startup code.

rust-highfive · 2017-09-28T20:16:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nikomatsakis · 2017-09-28T20:18:30Z

At first glance -- can we add some sort of test here?

dkl · 2017-09-28T21:46:30Z

I'm new to rustc and its test suite, but I got a codegen test like the following working:

fn main() {
}
// CHECK: define i32 @main(i32, i8**)

Is that ok/the right way to do it?

nagisa · 2017-09-29T11:02:13Z

Yes, that is fine.

nagisa · 2017-09-29T11:12:03Z

 }

+ pub fn c_int(ccx: &CrateContext) -> Type {
+ Type::i32(ccx)


This doesn’t seem like a particularly good implementation of c_int. That being said, liblibc defines type c_int = i32 for everything, so it is fine, I guess?

Yea, my thoughts exactly. It should be derived from the target context, as done for Type::isize() with target_pointer_width. But I thought that's not needed yet, because libc currently always uses i32 anyways. (does rustc support target platforms with 16bit/64bit C int?)

AVR uses a 16-bit int. I'll leave a comment on the tracking issue so this gets fixed if/when that gets upstreamed.

And MSP430 also. We currently support that target, but I don't think that needs to block this PR since this is strictly better than what we're already doing. (using i64).

isize is 16 bit on MSP:
rust/src/librustc_back/target/msp430_none_elf.rs
Line 18 in b7041bf
target_pointer_width:"16".to_string(),
So this PR would bump the argument from i16 to i32. The registers in MSP are 16-bits, and arguments are passed in registers, so this will most likely break the calling convention.

Ah, I missed that. Going to work on a fix.

nagisa · 2017-09-29T11:20:43Z

r=me with the codegen test as seen few comments above.

nagisa · 2017-09-29T21:18:06Z

@bors r+

bors · 2017-09-29T21:18:07Z

📌 Commit 6c3f190 has been approved by nagisa

nagisa · 2017-09-30T09:08:35Z

@bors r-

Until MSP/AVR/c_int situation is figured out. It might regress some targets.

(all i32 for now, as in liblibc)

Fix regression from c2fe69b, where main() signature was changed from using 16bit isize to 32bit c_int for argc parameter/result.

dkl · 2017-09-30T14:55:41Z

I've extended the target specifications with a field for c_int width, and set it to i16 for MSP. The same can probably be done for AVR.

nagisa · 2017-09-30T17:51:42Z

Some tests have failed:

[00:50:13] failures:
[00:50:13] [00:50:13] ---- [run-make] run-make/target-specs stdout ----
[00:50:13] [00:50:13] error: make failed
[00:50:13] status: exit code: 2
[00:50:13] command: "make"
[00:50:13] stdout:
[00:50:13] ------------------------------------------
[00:50:13] make[1]: Entering directory '/checkout/src/test/run-make/target-specs'
[00:50:13] LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/target-specs.stage2-x86_64-unknown-linux-gnu:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib:" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/target-specs.stage2-x86_64-unknown-linux-gnu -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/target-specs.stage2-x86_64-unknown-linux-gnu foo.rs --target=my-awesome-platform.json --crate-type=lib --emit=asm
[00:50:13] Makefile:3: recipe for target 'all' failed
[00:50:13] make[1]: Leaving directory '/checkout/src/test/run-make/target-specs'
[00:50:13] [00:50:13] ------------------------------------------
[00:50:13] stderr:
[00:50:13] ------------------------------------------
[00:50:13] make[1]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule.
[00:50:13] error: Error loading target specification: Field target-c-int-width in target specification is required
[00:50:13] |
[00:50:13] = help: Use `--print target-list` for a list of built-in targets
[00:50:13] [00:50:13] make[1]: *** [all] Error 101
[00:50:13] [00:50:13] ------------------------------------------
[00:50:13] [00:50:13] thread '[run-make] run-make/target-specs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2433:8
[00:50:13] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:50:13] [00:50:13] [00:50:13] failures:
[00:50:13] [run-make] run-make/target-specs

You’ll need to adjust test to specify target-c-int-width field.

nagisa · 2017-09-30T21:48:38Z

@bors r+

bors · 2017-09-30T21:48:39Z

📌 Commit a4e8373 has been approved by nagisa

bors · 2017-10-01T00:43:38Z

⌛ Testing commit a4e8373 with merge cfd927906b2fe00103f898ffc466d8b5c84e3cb2...

bors · 2017-10-01T01:41:39Z

💔 Test failed - status-appveyor

nagisa · 2017-10-01T09:14:49Z

@bors retry

failures:
---- [run-make] run-make\issue-26092 stdout ----
error: make failed
status: exit code: 2
command: "make"
stdout:
------------------------------------------
PATH="/c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu:C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-tools/x86_64-pc-windows-gnu/release/deps:/c/projects/rust/build/x86_64-pc-windows-gnu/stage0-sysroot/lib/rustlib/x86_64-pc-windows-gnu/lib:/c/Program Files (x86)/Inno Setup 5:/c/Python27:/c/projects/rust/mingw64/bin:/usr/bin:/c/Perl/site/bin:/c/Perl/bin:/c/Windows/system32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0:/c/Program Files/7-Zip:/c/Program Files/Microsoft/Web Platform Installer:/c/Tools/GitVersion:/c/Tools/PsTools:/c/Program Files/Git LFS:/c/Program Files (x86)/Subversion/bin:/c/Program Files/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/110/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn:/c/Program Files/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/Tools/Binn/ManagementStudio:/c/Tools/WebDriver:/c/Program Files (x86)/Microsoft SDKs/TypeScript/1.4:/c/Program Files (x86)/Microsoft Visual Studio 12.0/Common7/IDE/PrivateAssemblies:/c/Program Files (x86)/Microsoft SDKs/Azure/CLI/wbin:/c/Ruby193/bin:/c/Tools/NUnit/bin:/c/Tools/xUnit:/c/Tools/MSpec:/c/Tools/Coverity/bin:/c/Program Files (x86)/CMake/bin:/c/go/bin:/c/Program Files/Java/jdk1.8.0/bin:/c/Python27:/c/Program Files/nodejs:/c/Program Files (x86)/iojs:/c/Program Files/iojs:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/MSBuild/14.0/Bin:/c/Tools/NuGet:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/CommonExtensions/Microsoft/TestWindow:/c/Program Files/Microsoft DNX/Dnvm:/c/Program Files/Microsoft SQL Server/Client SDK/ODBC/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/Tools/Binn:/c/Program Files (x86)/Microsoft SQL Server/130/DTS/Binn:/c/Program Files/Microsoft SQL Server/130/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/110/DTS/Binn:/c/Program Files (x86)/Microsoft SQL Server/120/DTS/Binn:/c/Program Files (x86)/Apache/Maven/bin:/c/Python27/Scripts:/c/Tools/NUnit3:/c/Program Files/Mercurial:/c/Program Files/LLVM/bin:/c/Program Files/dotnet:/c/Program Files/erl8.3/bin:/c/Tools/curl/bin:/c/Program Files/Amazon/AWSCLI:/c/Program Files (x86)/Microsoft SQL Server/140/DTS/Binn:/c/Program Files (x86)/Microsoft Visual Studio 14.0/Common7/IDE/Extensions/Microsoft/SQLDB/DAC/140:/c/Program Files (x86)/Yarn/bin:/c/Program Files/Git/cmd:/c/Program Files/Git/usr/bin:/c/ProgramData/chocolatey/bin:/c/Tools/vcpkg:/c/Program Files (x86)/nodejs:/c/Program Files/Microsoft Service Fabric/bin/Fabric/Fabric.Code:/c/Program Files/Microsoft SDKs/Service Fabric/Tools/ServiceFabricLocalClusterManager:/c/Users/appveyor/AppData/Local/Yarn/bin:/c/Users/appveyor/AppData/Roaming/npm:/c/Program Files/AppVeyor/BuildAgent:/c/projects/rust:/c/projects/rust/handle" 'C:\projects\rust\build\x86_64-pc-windows-gnu\stage2\bin\rustc.exe' --out-dir /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu -L /c/projects/rust/build/x86_64-pc-windows-gnu/test/run-make/issue-26092.stage2-x86_64-pc-windows-gnu -o "" blank.rs 2>&1 | \
grep -i 'No such file or directory'
------------------------------------------
stderr:
------------------------------------------
make: *** [Makefile:4: all] Error 1
------------------------------------------
thread '[run-make] run-make\issue-26092' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:2433:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
[run-make] run-make\issue-26092
test result: FAILED. 160 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

Failure seems potentially spurious to me? cc @alexcrichton @Mark-Simulacrum

bors · 2017-10-01T09:15:01Z

⌛ Testing commit a4e8373 with merge 0defa20...

Fix native main() signature on 64bit Hello, in LLVM-IR produced by rustc on x86_64-linux-gnu, the native main() function had incorrect types for the function result and argc parameter: i64, while it should be i32 (really c_int). See also #20064, #29633. So I've attempted a fix here. I tested it by checking the LLVM IR produced with --target x86_64-unknown-linux-gnu and i686-unknown-linux-gnu. Also I tried running the tests (`./x.py test`), however I'm getting two failures with and without the patch, which I'm guessing is unrelated.

bors · 2017-10-01T12:23:17Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 0defa20 to master...

aidanhs · 2017-10-03T10:57:06Z

Spurious failure issue: #43402

dkl added 2 commits September 28, 2017 22:06

rustc: Add Type::c_int()
6c9a4ba
Add c_int for use in the compiler, assuming i32 for all targets as in libc.

rust-highfive assigned nikomatsakis Sep 28, 2017

test: Check native main() signature
6c3f190

nagisa reviewed Sep 29, 2017
View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 29, 2017

nagisa added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 29, 2017

This was referenced Sep 29, 2017
RFC: Merging the avr-rust fork upstream #44052
Closed
Support 16-bit int in main() signature #44934
Closed

nagisa added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 30, 2017

dkl added 2 commits September 30, 2017 15:54

rustc: Specify c_int width for each target
a204c19
(all i32 for now, as in liblibc)

rustc: Use 16bit c_int for msp430
12a8bc6
Fix regression from c2fe69b, where main() signature was changed from using 16bit isize to 32bit c_int for argc parameter/result.

test: Update target specs test for new target-c-int-width field
a4e8373

bors merged commit a4e8373 into rust-lang:masterOct 1, 2017

phil-opp mentioned this pull request Oct 3, 2017
Add target-c-int-width in x86_64-blog_os.json phil-opp/blog_os#353
Closed

pnkfelix mentioned this pull request Oct 3, 2017
MIR borrowck: move span_label to borrowck_errors.rs#44922
Merged

phil-opp mentioned this pull request Oct 3, 2017
Add new required target-c-int-width field to target json phil-opp/blog_os#354
Merged

Uh oh!

Uh oh!

Conversation

dkl commented Sep 28, 2017

Uh oh!

rust-highfive commented Sep 28, 2017

Uh oh!

nikomatsakis commented Sep 28, 2017

Uh oh!

dkl commented Sep 28, 2017

Uh oh!

nagisa commented Sep 29, 2017

Uh oh!

nagisaSep 29, 2017

Choose a reason for hiding this comment

Uh oh!

dklSep 29, 2017

Choose a reason for hiding this comment

Uh oh!

matticoSep 29, 2017

Choose a reason for hiding this comment

Uh oh!

matticoSep 29, 2017

Choose a reason for hiding this comment

Uh oh!

nagisaSep 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dklSep 30, 2017

Choose a reason for hiding this comment

Uh oh!

nagisa commented Sep 29, 2017

Uh oh!

nagisa commented Sep 29, 2017

Uh oh!

bors commented Sep 29, 2017

Uh oh!

nagisa commented Sep 30, 2017

Uh oh!

dkl commented Sep 30, 2017

Uh oh!

nagisa commented Sep 30, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagisa commented Sep 30, 2017

Uh oh!

bors commented Sep 30, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

nagisa commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

bors commented Oct 1, 2017

Uh oh!

aidanhs commented Oct 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nagisaSep 30, 2017•
edited
Loading

nagisa commented Sep 30, 2017•
edited
Loading