-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
[ltsmaster] Backport PR #2315. #2334
Conversation
|
@redstar As you're busy with lts master, is this PR still valuable? Or should we just drop debuginfo support in lts (because we only use it as bootstrapper)? I reckon debuginfo may help with fixing bugs but... |
|
Yes, I consider this PR still valueable. One of my next tasks. I like to have ltsmaster in a good, clean state. |
|
@JohanEngelen I do not get the test failure ( |
The change is to fix debuginfo build errors with Phobos and LLVM5.0. This PR is mostly copy-pasting from master and fixing compile errors, so not much checking whether things make sense or not for LTS.
2456bcf
to
8993eb7
Compare
|
@redstar done |
|
I now managed to reproduce the error on one system. |
| IF_LOG { | ||
| Logger::cout() << "Addr: " << *val << '\n'; | ||
| Logger::cout() << "of type: " << *val->getType() << '\n'; | ||
| } | ||
| if (byref || (vd->isParameter() && getIrParameter(vd)->arg && | ||
| getIrParameter(vd)->arg->byref)) { | ||
| const bool isRefOrOut = vd->isRef() || vd->isOut(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluation of isRefOrOut and vd->isParameter... gives different results and as consequence a missing load, leading to the failure in the test suite. I think a simple solution is to add
const bool isRefParameter = vd->isParameter() && getIrParameter(vd)->arg && getIrParameter(vd)->arg->byref;
and change the condition below to
} else if (byref || isRefOrOut || isRefParameter) {
This makes sure that no load is missed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redstar you can push to this branch (afaik), can you add your fix?
|
I merged your commit. |
The change is to fix debuginfo build errors with Phobos and LLVM5.0. This PR is mostly copy-pasting from master and fixing compile errors, so not much checking whether things make sense or not for LTS.