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

[LLVM 4.0] Handle new DIFlags enum #37857

Merged
merged 2 commits into from
Dec 4, 2016
Merged

Conversation

shepmaster
Copy link
Member

No description provided.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@shepmaster
Copy link
Member Author

@rkruppe - You'll note that I took a few pieces out that weren't directly related to DINode, those have been moved to separate commits.

@shepmaster shepmaster mentioned this pull request Nov 18, 2016
23 tasks
@alexcrichton
Copy link
Member

r? @michaelwoerister

#if LLVM_VERSION_GE(4, 0)
auto Flags = DINode::FlagPrototyped;
#else
unsigned Flags = DINode::FlagPrototyped;
Copy link
Contributor

@hanna-kruppe hanna-kruppe Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we avoid these version checks by just unconditionally doing auto Flags = ...;? (here and in the rest of this file)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, yes, but I wanted to keep it consistent with the majority case of FlagZero, which I think needs to be different.

#if LLVM_VERSION_GE(4, 0)
auto Flags = DINode::FlagZero;
#else
unsigned Flags = 0;
Copy link
Contributor

@hanna-kruppe hanna-kruppe Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the DINode::FlagFoo names seem to exist in LLVM 3.7 so why not just use those instead of magic numbers?

Edit: Nevermind, DINode::FlagZero is the one exception that didn't exist in 3.7 =/ I'd still like a name for that magic number.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, could you point me to where FlagZero is defined? I was under the impression that that value did not exist until the enum change. Previously, it was just hardcoded as 0.

Copy link
Contributor

@hanna-kruppe hanna-kruppe Nov 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, it did not exist, see edit above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replaced this with a single #ifdef and removed the conditions from each case. Check it out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it.

@michaelwoerister
Copy link
Member

Thanks for the PR!
I think it would be better to keep exposing the flags to Rust though. For example, if we want to fix #9228, we need to have this functionality available. Could we follow a similar pattern as with LLVMRustLinkage for example?

@shepmaster
Copy link
Member Author

shepmaster commented Nov 18, 2016

keep exposing the flags to Rust though

I was really, really afraid you'd say that. That enum business is terribly verbose (and annoying).

Not to say I'm not going to, I'm just whinging.

@shepmaster
Copy link
Member Author

keep exposing the flags to Rust though

Oh right, I remember why this is more difficult than LLVMRustLinkage; these are actually bitflags, not an enum. Is there an example of marshalling such a type already?

@michaelwoerister
Copy link
Member

I was really, really afraid you'd say that. That enum business is terribly verbose (and annoying).

I'm sorry, I know :)

I would probably use the bitflags! macro on the Rust side (so I get something type safe in Rust that also has a predictable C-representation) and then do a mapping on the C++ side like with LLVMRustLinkage so we don't depend on LLVM not changing values.

@shepmaster
Copy link
Member Author

shepmaster commented Nov 18, 2016

OK, this is going to conflict with #37831 because someone removed the bitflags crate that this commit is going to want to use. 😈 Hopefully we can get that submitted soon so I can re-add it.

@sanxiyn
Copy link
Member

sanxiyn commented Nov 21, 2016

#37831 landed.

@shepmaster
Copy link
Member Author

@michaelwoerister please give this another look; I've rebased and been able to compile the current master branch and the LLVM 4.0 branch is off compiling somewhere in stage1.

@shepmaster shepmaster changed the title [LLVM 4.0] Handle new DINode enum [LLVM 4.0] Handle new DIFlags enum Dec 1, 2016
@shepmaster
Copy link
Member Author

Ah, looks like I need to look at the various LLVM versions to see when each flag was introduced. I assume I should not include any flags that are not supported in the oldest version of LLVM we support, because otherwise we'd have no ability to do anything with those flags with those older versions.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'll be happy to r+ after you've addressed the nits.

bitflags! {
#[repr(C)]
#[derive(Debug, Default)]
flags DIFlags: ::libc::c_uint {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's uint32_t on the C++ side, we should use libc::uint32_t here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


if (is_set(flags & LLVMRustDIFlags::FlagPrivate)) { result |= DINode::DIFlags::FlagPrivate; }
if (is_set(flags & LLVMRustDIFlags::FlagProtected)) { result |= DINode::DIFlags::FlagProtected; }
if (is_set(flags & LLVMRustDIFlags::FlagPublic)) { result |= DINode::DIFlags::FlagPublic; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the visibility bits are not really flags, but rather a 2-bit number, this is not quite correct.

private   = 01
protected = 10
public    = 11

The result will be correct, but it I'm not sure that that would hold if they changed the encoding on the LLVM side. Something like the following would be a better conceptual fit:

switch flags & 0x3 {
    LLVMRustDIFlags::FlagPrivate: result |= DINode::DIFlags::FlagPrivate; break;
    LLVMRustDIFlags::FlagProtected: result |= DINode::DIFlags::FlagProtected; break;
    LLVMRustDIFlags::FlagPublic: result |= DINode::DIFlags::FlagPublic; break;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch (flags & 0x3) { is not possible because I've attempted to use a typesafe enum (enum class). Would you like me to discard that and move to a traditional plain enum?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could do this though:

+inline LLVMRustDIFlags visibility(LLVMRustDIFlags f) {
+    return static_cast<LLVMRustDIFlags>(static_cast<uint32_t>(f) & 0x3);
+}
+

+    switch (visibility(flags)) {
+    case LLVMRustDIFlags::FlagPrivate:
+        result |= DINode::DIFlags::FlagPrivate;
+        break;
+    case LLVMRustDIFlags::FlagProtected:
+        result |= DINode::DIFlags::FlagProtected;
+        break;
+    case LLVMRustDIFlags::FlagPublic:
+        result |= DINode::DIFlags::FlagPublic;
+        break;
+    default:
+        // The rest are handled below
+        break;
+    }
+

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

In LLVM 4.0, this enum becomes an actual type-safe enum, which breaks
all of the interfaces. Introduce our own copy of the bitflags that we
can then safely convert to the LLVM one.
@michaelwoerister
Copy link
Member

@bors r+

Thanks a lot, @shepmaster! I know this can be tedious stuff but future generations of Rust compiler hackers will thank you :)

@bors
Copy link
Contributor

bors commented Dec 3, 2016

📌 Commit 757a9ce has been approved by michaelwoerister

@bors
Copy link
Contributor

bors commented Dec 4, 2016

⌛ Testing commit 757a9ce with merge 125474d...

bors added a commit that referenced this pull request Dec 4, 2016
@shepmaster
Copy link
Member Author

I know this can be tedious stuff but future generations of Rust compiler hackers will thank you :)

Aww, I know I'm grumbly, but in my heart of hearts I also know the code is better after the review. Besides, with the work we are doing for LLVM, it's not unusual for me to be that future hacker.

@bors bors merged commit 757a9ce into rust-lang:master Dec 4, 2016
@shepmaster shepmaster deleted the llvm-4.0-dinodes branch April 25, 2018 19:28
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

Successfully merging this pull request may close these issues.

8 participants