Discussion:
r177329 - This code works around what appears to be a bug in another part of clang.
(too old to reply)
Reed Kotler
2013-03-18 22:18:00 UTC
Permalink
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329

URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
Log:
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.


Modified:
cfe/trunk/lib/CodeGen/TargetInfo.cpp

Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
@@ -4323,7 +4323,8 @@ public:
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
Rafael Espíndola
2013-03-18 22:42:06 UTC
Permalink
Post by Reed Kotler
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
I don't think this is a reasonable change. If there is a bug somewhere
else, it should be fixed. This change just hides it. It doesn't even
include a testcase. Please revert. If some other patch caused problem,
revert it too.

Cheers,
Rafael
reed kotler
2013-03-18 23:29:08 UTC
Permalink
Post by Rafael Espíndola
Post by Reed Kotler
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
I don't think this is a reasonable change. If there is a bug somewhere
else, it should be fixed. This change just hides it. It doesn't even
include a testcase. Please revert. If some other patch caused problem,
revert it too.
Cheers,
Rafael
Hi Rafael,

I don't think I should have to revert this patch.

I'm not even sure if there is another bug.

I don't know the code that is calling this. It's just my opinion that
there is some other issue.

The other code is this .cpp is doing the same casting that I was doing;
that is where I got the code from. Originally I copied exactly what the
x86 was doing but on checkin was told to change it to it's current form
by Bill Wendling. This current form already appears exactly as my code
does for some other targets so they have the same problem potentially.

Everything works for mips now.

Someone that knows clang can explain how FD is a function declaration
but GV is not a Function.
Why is that my responsibility to sort that out?

CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");

This code is need for the mips test-suite to not regress and for my attribute work to continue.

Reed


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20130318/7c32c04b/attachment.html>
Rafael Espíndola
2013-03-18 23:36:32 UTC
Permalink
Post by reed kotler
Hi Rafael,
I don't think I should have to revert this patch.
I'm not even sure if there is another bug.
I don't know the code that is calling this. It's just my opinion that there
is some other issue.
Then you should investigate that. What we should never do is add

if (!Fn) return; // should not happen

If it should not happen, this should be an assert (as it was before
your patch). It it is a logically valid condition, instead of the
wrong comment we should have a test where Fn is null.
Post by reed kotler
Someone that knows clang can explain how FD is a function declaration but GV
is not a Function.
Why is that my responsibility to sort that out?
Because you changed the code.
Post by reed kotler
This code is need for the mips test-suite to not regress and for my
attribute work to continue.
Sorry. You should not push problems to others. Looks like you actually
have a testcase and you just need to reduce it. Please revert this
patch and reduce the testcase. Depending on what you find you can put
this patch back, but without the comment and with a testcase where Fn
is null.
Post by reed kotler
Reed
Cheers,
Rafael
reed kotler
2013-03-18 23:52:50 UTC
Permalink
Post by Rafael Espíndola
Post by reed kotler
Hi Rafael,
I don't think I should have to revert this patch.
I'm not even sure if there is another bug.
I don't know the code that is calling this. It's just my opinion that there
is some other issue.
Then you should investigate that. What we should never do is add
if (!Fn) return; // should not happen
If it should not happen, this should be an assert (as it was before
your patch). It it is a logically valid condition, instead of the
wrong comment we should have a test where Fn is null.
Post by reed kotler
Someone that knows clang can explain how FD is a function declaration but GV
is not a Function.
Why is that my responsibility to sort that out?
Because you changed the code.
Post by reed kotler
This code is need for the mips test-suite to not regress and for my
attribute work to continue.
Sorry. You should not push problems to others. Looks like you actually
have a testcase and you just need to reduce it. Please revert this
patch and reduce the testcase. Depending on what you find you can put
this patch back, but without the comment and with a testcase where Fn
is null.
Post by reed kotler
Reed
Cheers,
Rafael
THere are several patches that need to be reverted to set this back and
we will just break other code and you will impede work that I'm doing
that needs this.

I will look into what is causing this.

Give me some time to figure out what is causing this.

Please don't revert this. You will just make a lot of unnecessary work
for me and the Mips team.

My original patch for attributes did not have this issue and I changed
it to suite the reviewers.
I did things exactly how the x86 port did them and they avoid this issue
by luck or else maybe they
already found this problem and worked around it.

There is code already identical to this in this same .cpp for other targets.

We are talking about a two line patch.

You are punishing me for being honest about the situation.

I don't believe that I should be getting called in this situation with
those parameter values. Maybe I'm wrong. There is no documentation on
any of this so how would I know. Other code also assumes the same. I've
filed a bug against clang for this and duly noted the issue both in the
code and in the putback notes.

There are no mips target attributes other than mips16/nomips16 and those
are for functions.

This test case has no Mips target specific attributes at all so why is
this method even getting called?


Reed
reed kotler
2013-03-19 00:11:23 UTC
Permalink
I am working on trying to understand how I can be called with the
parameter values I am getting called with.

If it turns out to be okay, then i will remove the comment

// should not happen

I don't really know it "should not happen". I just believe this to be
the case.

If it turns out that these parameters should indeed not be being passed
for me, I will attempt to make a patch or else update the bug that I
already filed against clang for this issue.
Post by Rafael Espíndola
Post by reed kotler
Hi Rafael,
I don't think I should have to revert this patch.
I'm not even sure if there is another bug.
I don't know the code that is calling this. It's just my opinion that there
is some other issue.
Then you should investigate that. What we should never do is add
if (!Fn) return; // should not happen
If it should not happen, this should be an assert (as it was before
your patch). It it is a logically valid condition, instead of the
wrong comment we should have a test where Fn is null.
Post by reed kotler
Someone that knows clang can explain how FD is a function declaration but GV
is not a Function.
Why is that my responsibility to sort that out?
Because you changed the code.
Post by reed kotler
This code is need for the mips test-suite to not regress and for my
attribute work to continue.
Sorry. You should not push problems to others. Looks like you actually
have a testcase and you just need to reduce it. Please revert this
patch and reduce the testcase. Depending on what you find you can put
this patch back, but without the comment and with a testcase where Fn
is null.
Post by reed kotler
Reed
Cheers,
Rafael
David Blaikie
2013-03-19 00:18:10 UTC
Permalink
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?

Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.

If your reviewers told you to write it some way that doesn't work,
discuss it with them.

If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.

We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.

Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
reed kotler
2013-03-19 00:22:15 UTC
Permalink
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
With my fix, everything works. Reverting my change will cause failures
at Mips in our test-suite runs. Not a lot of failures but some
compilation failures with c++ exception handling it seems.

I am investigating this more thoroughly now to understand if my original
comment was incorrect, maybe it can be called with an functional
declaration and global value which is not function or maybe there is a
bug in clang.
reed kotler
2013-03-19 00:28:55 UTC
Permalink
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment

// should not happen

That was just my opinion that I should not be called under these conditions.
David Blaikie
2013-03-19 00:39:02 UTC
Permalink
Post by reed kotler
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment
// should not happen
With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.
My point would be not that this patch should be reverted, but that the
original patch that added the buggy code be reverted until the bug/fix
is properly understood.
Post by reed kotler
That was just my opinion that I should not be called under these conditions.
& that seems to indicate that you don't have a clear understanding of
the code that's been written/committed - that doesn't seem like code
we would want committed, does it?
reed kotler
2013-03-19 00:43:42 UTC
Permalink
Post by David Blaikie
Post by reed kotler
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment
// should not happen
With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.
My point would be not that this patch should be reverted, but that the
original patch that added the buggy code be reverted until the bug/fix
is properly understood.
Post by reed kotler
That was just my opinion that I should not be called under these conditions.
& that seems to indicate that you don't have a clear understanding of
the code that's been written/committed - that doesn't seem like code
we would want committed, does it?
I'm going to remove the comment and supply the test case.

In the case where I am passed a Function Declaration and Global Value
which is not convertible to a Function, there is nothing for me to do
here. There are no mips attributes which could be processed in this code
that do not conform to that.


Reed
Rafael Espíndola
2013-03-19 14:36:48 UTC
Permalink
Post by reed kotler
I'm going to remove the comment and supply the test case.
Good. I have reverted the patch while that happens. I understand you
might have a crash that blocks you, but that is not part of how our
code review process works.

Just having a test with a fixme saying something on the lines of: this
should probably be handled differently is OK.

If you don't even have time to reduce a testcase, please keep changes
like this in a local branch.

So, about the testcase. It is the one in llvm.org/pr15538 that needs
reducing, right? If that is so I am more than happy to help you reduce
it and create a testcase we can add to clang.
Post by reed kotler
In the case where I am passed a Function Declaration and Global Value which
is not convertible to a Function, there is nothing for me to do here. There
are no mips attributes which could be processed in this code that do not
conform to that.
Reed
Cheers,
Rafael
Rafael Espíndola
2013-03-19 14:50:08 UTC
Permalink
Post by Rafael Espíndola
So, about the testcase. It is the one in llvm.org/pr15538 that needs
reducing, right? If that is so I am more than happy to help you reduce
it and create a testcase we can add to clang.
It reduces to:

struct B {
B();
};
B::B() {
}

The problem is that you are being passed an alias because of
-mconstructor-aliases. Since aliases don't have attributes, the
correct fix is probably to not call SetTargetAttributes since there is
nothing for it to set attributes on.

Cheers,
Rafael
reed kotler
2013-03-19 15:02:39 UTC
Permalink
Post by Rafael Espíndola
Post by Rafael Espíndola
So, about the testcase. It is the one in llvm.org/pr15538 that needs
reducing, right? If that is so I am more than happy to help you reduce
it and create a testcase we can add to clang.
struct B {
B();
};
B::B() {
}
The problem is that you are being passed an alias because of
-mconstructor-aliases. Since aliases don't have attributes, the
correct fix is probably to not call SetTargetAttributes since there is
nothing for it to set attributes on.
Cheers,
Rafael
Okay. thanks for the help.

I will take care of this today.

Reed
Rafael Espíndola
2013-03-19 15:07:00 UTC
Permalink
Post by reed kotler
Okay. thanks for the help.
I will take care of this today.
Fixed in r177402. Please put a bit more effort into testing and
understanding the problems next time. This thread took more effort
than the bug it is about :-(
Post by reed kotler
Reed
Cheers,
Rafael
reed kotler
2013-03-19 15:15:54 UTC
Permalink
Post by Rafael Espíndola
Post by reed kotler
Okay. thanks for the help.
I will take care of this today.
Fixed in r177402. Please put a bit more effort into testing and
understanding the problems next time. This thread took more effort
than the bug it is about :-(
Post by reed kotler
Reed
Cheers,
Rafael
I appreciate all the help.

I understand what you want in the future as to how to handle these
situations in the code when they arise and I will do my best to adhere
to it.

I would like to then add an llvm_unreachable in our code to make sure
that I don't again see something unexpected.

So I'm not expecting to see a Function Declaration and then a Global
Value that is not a function when attributes are being propagated.

I will file a bug against this whole file TargetLowering.cpp. There is
similar cleanup needed everywhere for several issues that I've found
while adding the Mips attributes.

Reed
reed kotler
2013-03-19 18:26:26 UTC
Permalink
Post by reed kotler
Post by Rafael Espíndola
Post by reed kotler
Okay. thanks for the help.
I will take care of this today.
Fixed in r177402. Please put a bit more effort into testing and
understanding the problems next time. This thread took more effort
than the bug it is about :-(
Post by reed kotler
Reed
Cheers,
Rafael
I appreciate all the help.
I understand what you want in the future as to how to handle these
situations in the code when they arise and I will do my best to adhere
to it.
I would like to then add an llvm_unreachable in our code to make sure
that I don't again see something unexpected.
So I'm not expecting to see a Function Declaration and then a Global
Value that is not a function when attributes are being propagated.
I will file a bug against this whole file TargetLowering.cpp. There is
similar cleanup needed everywhere for several issues that I've found
while adding the Mips attributes.
Reed
I understand now your reason to leave the cast in there as opposed to a
dyn_cast and a check.

I think this case is closed.

Thanks for all the help.

Reed

reed kotler
2013-03-19 01:13:26 UTC
Permalink
This situation occurs at the end of

bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl,
GlobalDecl TargetDecl) {

apparently you can transfer attributes to an alias.

// Finally, set up the alias with its proper name and attributes.
SetCommonAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);

Nobody else is processing calls with such value pairs.

There are no mips target attributes in this example put apparently it is
calling these target independent attribute processing methods anyway.

Perhaps I should do something which such calls but that would be a
separate patch with it's own test case.

In this case, for sure I should be doing nothing because they are not
even mips attributes.
Post by David Blaikie
Post by reed kotler
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment
// should not happen
With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.
My point would be not that this patch should be reverted, but that the
original patch that added the buggy code be reverted until the bug/fix
is properly understood.
Post by reed kotler
That was just my opinion that I should not be called under these conditions.
& that seems to indicate that you don't have a clear understanding of
the code that's been written/committed - that doesn't seem like code
we would want committed, does it?
reed kotler
2013-03-19 01:48:08 UTC
Permalink
I have proposed a patch to clang in
http://llvm.org/bugs/show_bug.cgi?id=15538

Do you want me to submit this patch?

For now I will make the test case for my patch and remove the offending
comment.
Post by David Blaikie
Post by reed kotler
Post by David Blaikie
Post by Reed Kotler
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
This code works around what appears to be a bug in another part of clang.
I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
This code is safer anyway because "cast" assumes you really know that
it's okay to make the cast. In this case isa should not be false and
dyn_cast should not return null as far as I understand. But everything
else is valid so I did not want to revert my previous patch for attributes
mips16/nomips16 or use an llvm_unreachable here which would make a number
of our tests fail for mips.
cfe/trunk/lib/CodeGen/TargetInfo.cpp
Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
CodeGen::CodeGenModule &CGM) const {
const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
if (!FD) return;
- llvm::Function *Fn = cast<llvm::Function>(GV);
+ llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+ if (!Fn) return; // should not happen
Do you have a test case to commit with this?
Also - I tend to agree with Rafael: if you don't know why you need to
make a change, then it's probably not OK to make this fix. Sounds like
you don't understand what's going on here & are papering over it in a
way that seems to work but without any basis to believe that your
change is right/correct other than "it works". We don't really develop
code this way.
If your reviewers told you to write it some way that doesn't work,
discuss it with them.
If other targets have the same bug, that's not necessarily sufficient
justification to add another instance of the same bug once we know it
is a bug.
We aren't punishing you for being honest, we're asking you to not
commit buggy/insufficiently understood/justified code.
Checking in an uncertain fix while you investigate the problem isn't
the usual practice on the project - we tend to revert a patch until we
can it's correct. (granted, if a bug has been in the codebase long
enough we don't trawl back through the commits & rip out the
patch/functionality that added it - so, yes, the longer a patch is in
the more likely that when we find a bug we'll just let the tree
continue to be broken until we commit a fix for the bug - in this case
it sounds like your contribution is still fresh enough to be a fix
(with a known correct fix) or revert kind of situation)
Post by Reed Kotler
if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
The fix may be to just eliminate the comment
// should not happen
With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.
My point would be not that this patch should be reverted, but that the
original patch that added the buggy code be reverted until the bug/fix
is properly understood.
Post by reed kotler
That was just my opinion that I should not be called under these conditions.
& that seems to indicate that you don't have a clear understanding of
the code that's been written/committed - that doesn't seem like code
we would want committed, does it?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20130318/b86bbe9c/attachment-0001.html>
Loading...