This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][RISCV] Fix risc-v target build
ClosedPublic

Authored by Emmmer on Aug 11 2022, 4:08 AM.

Details

Summary

Fixed an inconsistency between D130985 and D130342

This should be a follow-up of D130985

Diff Detail

Event Timeline

Emmmer created this revision.Aug 11 2022, 4:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 4:08 AM
Emmmer requested review of this revision.Aug 11 2022, 4:08 AM
Emmmer edited the summary of this revision. (Show Details)Aug 11 2022, 4:09 AM

(please submit simple build fixes first and have them reviewed post commit, so that the bots don't stay red for long.)

Sorry, I didn't get "have them reviewed post commit", I'm new to llvm so can you give me some instruction on how to do it? Thank you.

Just commit the change, and if there are comments on the commit later, address then in a follow-up.

DavidSpickett accepted this revision.Aug 11 2022, 5:50 AM

(please submit simple build fixes first and have them reviewed post commit, so that the bots don't stay red for long.)

And in this case the way I'd review it is to check it all out and build it so it amounts to the same thing (and saves me firing up a Windows machine).

LGTM regardless.

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp
54

On the assumption that riscv6 linux doesn't have a 32 bit compatibility mode, this is fine.

(that's what this is trying to detect, for example running arm32 on arm64).

This revision is now accepted and ready to land.Aug 11 2022, 5:50 AM

Can this land, please? The windows build has been broken for close to 7 hours now.

Emmmer updated this revision to Diff 451829.Aug 11 2022, 6:08 AM

Can this land, please? The windows build has been broken for close to 7 hours now.

After hours of trying to set up a working windows dev machine, I failed to reproduce the build failure you provided here (http://45.33.8.238/win/64255/step_4.txt). I am using a Windows 10 21H2 19044.1889 with VS 2022 and MSVC 143.

I guess it is the mocked dwarf PC register number that was converted to signed int32 and then implicitly converted to unsigned int32 where msvc reject this kind of conversion. You may need to check manually if this was the fix you want ( sorry for that :( ). If so, we land it immediately.

That bot uses clang-cl as compiler.

I can't easily check it either at the moment. It's easiest to land this and see if it helps.

Changing enum to enum : unsigned might (or might not) help as well.

That bot uses clang-cl as compiler.

I can't easily check it either at the moment. It's easiest to land this and see if it helps.

Changing enum to enum : unsigned might (or might not) help as well.

We take a lot risk to land it without test. What if it does not help? Adding more commits until it is fixed?

It's _already broken_. There's zero risk to make it worse. (Assuming you checked it still builds on Linux.)

Yes, if it doesn't help you'd land another fix attempt.

(This is what the "if it takes a while" was for, though.)

okay, let's try

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2022, 6:43 AM

This helped, thanks.

labath added inline comments.Aug 16 2022, 8:37 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_riscv64.cpp
54

(even if it does, I don't think we have to support that mode right away)