Support for the program data address space option of LLVM's Target Datalayout

[TimNN]

This was introduced recently (specifically, for AVR, cc @dylanmckay).

(I came up with this when attempting to run avr-rust rebased on the latest rust-lang commits. If this requires a different design, some additional discussions, or is not something to pursue right now, I'd be happy to close this PR).

Note that this somewhat overlaps with @DiamondLovesYou's #51576, I think, although the implementation here is significantly simpler: Since the address space applies to all program data, we can just check the pointee's type whenever we create an LLVM pointer type. If it is a function we use the program data address space; if not we use the default address space.

cc @eddyb, who has been reviewing #51576

Ref: https://llvm.org/docs/LangRef.html#data-layout

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[rust-highfive]

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:44] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:44] tidy error: /checkout/src/librustc_target/abi/mod.rs:109: line longer than 100 chars
[00:04:45] some tidy checks failed
[00:04:45] [00:04:45] [00:04:45] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:45] [00:04:45] [00:04:45] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:45] Build completed unsuccessfully in 0:00:47
[00:04:45] Build completed unsuccessfully in 0:00:47
[00:04:45] make: *** [tidy] Error 1
[00:04:45] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0cdda034
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:02bd36e4:start=1539273876699334755,finish=1539273876704878228,duration=5543473
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01dbbf09
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0531b499
travis_time:start:0531b499
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:19dc590e
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[nikomatsakis]

It is possible to "mis-use" this, I assume, for example via transmutes or something? Can we wind up with "valid Rust code" giving weird LLVM assertion failures or other errors?

[TimNN]

That's a good point. I'll see what happens if I transmute a few things to a function pointer.

[nikomatsakis]

cc @rust-lang/compiler — seems like an ad-hoc extension, nominating for brief T-compiler discussion to decide best path

[TimNN]

seems like an ad-hoc extension

Yeah, this is definitely a bit ad-hoc, however overall the approach doesn't seem too bad to me. In any case, I came up with this while trying to get a target to work that isn't even officially supported yet. I'd be totally happy if you said this requires some more design work in the future.

[nikomatsakis]

@TimNN to be clear, I didn't mean "ad hoc" in the pejorative sense. Just that it's one of those "small features" that we often decide without an RFC -- it might however warrant a feature gate or a tracking issue or some such thing.

[DiamondLovesYou]

My patch can be easily extended to support this. But I think the space should be called the instruction address space, since "program memory" is kinda ambiguous (even though this is the term LLVM uses).

@eddyb wanted any value to be cast into the flat address space before that value is used (ie uses of a global would actually use an addrspacecast-ed const expr), in order to reduce the patch's complexity footprint. @TimNN would this be possible for the AVR target machine? The initial version I've whipped up doesn't force this backflip (it uses the correct space for functions when creating LLVM types from our types), but @eddyb might want them to go through a flat space cast round trip.

[TimNN]

think the space should be called the instruction address space

I'd be fine with that, as you noticed I was just copying LLVM terminology.

@eddyb wanted any value to be cast into the flat address space before that value is used (ie uses of a global would actually use an addrspacecast-ed const expr), in order to reduce the patch's complexity footprint. @TimNN would this be possible for the AVR target machine?

I don't know. I understand how my patch works but overall I'm mostly unfamiliar with the involved concepts. IIRC, however, LLVM was unhappy casting a function to a function pointer in an incorrect addrspace (In case that information helps you).

---

In any case, if you think that the functionality added here could be better supported with your patch, I'd be happy to wait for that to be merged. (I'd probably open this PR against the avr-rust repo for others to use in the meantime, but for me it is in no way urgent to get this submitted).

[bors]

☔ The latest upstream changes (presumably #55171) made this pull request unmergeable. Please resolve the merge conflicts.

[eddyb]

I really want to avoid us introspecting LLVM types. I think we should either not use non-flat address space pointers or we should special-case fn pointers - from a quick search, I found only 2 places that create them:

rust/src/librustc_codegen_llvm/type_of.rs

Line 268 in 40123a1

FnType::new(cx, sig,&[]).llvm_type(cx).ptr_to()

rust/src/librustc_codegen_llvm/meth.rs

Line 42 in 40123a1

let llvtable = bx.pointercast(llvtable, fn_ty.llvm_type(bx.cx).ptr_to().ptr_to());

Since these are both calling .ptr_to() on the result of FnType::llvm_type, a helper method on FnType can probably be added.

[nikomatsakis]

I approve

[eddyb]

Suggested change

	[ref p]if p.starts_with("P") =>
	[p]if p.starts_with("P") =>

[DiamondLovesYou]

@TimNN Well, I've push a change to support the feature needed in this PR, so feel free to give it a try.

I don't know. I understand how my patch works but overall I'm mostly unfamiliar with the involved concepts. IIRC, however, LLVM was unhappy casting a function to a function pointer in an incorrect addrspace (In case that information helps you).

It does help. As I said, my patch put the vtable entries and all declared functions in the instruction address space, so that should work. I just realized it would not, however, do the same for function ptrs cast from non fn ptr values, like integers (perhaps a bit of an edge case). Currently, it tries to cast the value to the flat address space just like any other ptr.

But, baring that, it should work for AVR with a suitable target spec. If you'd like to give my patch a shot, let me know how it goes!

I would definitely prefer to have my patch merged instead; it (now) supports (that int -> fn ptr issue not withstanding) this address space, and can compile Rust which then runs on an AMDGPU.

[TimNN]

Thanks for your reviews, @eddyb & @nikomatsakis!

I'm not sure what you think, but I feel like @DiamondLovesYou's PR #51576 is the way to go in the long term.

I however don't know what the timeline would be to get that merged (or if you maybe have concerns that would prevent merging it).

I see two ways forward:

1. close this for now until a decision on Make librustc_codegen_llvm aware of LLVM address spaces. #51576 is reached.
2. merge this for now (after addressing your comments) and refactor / upgrade things in Make librustc_codegen_llvm aware of LLVM address spaces. #51576.

What do you all think?

[shepmaster]

A drive-by comment from AVR-land...

We think that users would like / need the ability to annotate certain pieces of memory with which address space they should go in. We opened an issue about this, but in short, something like this strawman syntax could address the issue:

#[space(progmem)]pubstaticFOOBAR:&'staticstr = "hello world";

I don't know that this PR would handle something like that, but it may be worth keeping in mind for the underlying design.

[TimNN]

I've updated this PR to address the review comments. However I'll close this for now until there's a decision on ##51576.

[rust-highfive]

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[bors]

⌛ Testing commit 50a2d47 with merge f705d13c0de0f6721468e867ae993bbf819ddbbd...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry

3 hour timeout in x86_64-msvc, likely due to LLVM cache purge.

[bors]

⌛ Testing commit 50a2d47 with merge b3a9ad420ecc2b738cc66a2437a865ec6f6f648d...

[bors]

💔 Test failed - status-appveyor

[TimNN]

@bors retry

• Another timeout on AppVeyor due to LLVM rebuilding?

[bors]

⌛ Testing commit 50a2d47 with merge 9b8f902...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 9b8f902 to master...