This is an archive of the discontinued LLVM Phabricator instance.

Call attention to strange case in getMultiarchTriple, and DRY so much.
ClosedPublic

Authored by dougk on Jun 22 2015, 9:32 AM.

Details

Summary

There is a suspicious fallthrough at ppc64 which on first glance seems unlikely to be right, but also seems unlikely to cause a problem, and remains as-is but with a comment. It seems like it should have resembled the pattern of the cases above it:
if (test) return "/some/path"; return TargetTriple.str();
whereas it actually fell into the ppc64le case.
I am not sure what the intent was, so this patch adds an explicit "// Fallthrough" comment.

Additionally there is a uniform catch-all case at the end rather than having every architecture recognize both a special case (or two) and the general case.

Diff Detail

Repository
rL LLVM

Event Timeline

dougk updated this revision to Diff 28121.Jun 22 2015, 9:32 AM
dougk retitled this revision from to Call attention to strange case in getMultiarchTriple, and DRY so much..
dougk updated this object.
dougk edited the test plan for this revision. (Show Details)
dougk added reviewers: wschmidt, chandlerc.
dougk added a subscriber: Unknown Object (MLST).
chandlerc accepted this revision.Jun 26 2015, 1:09 AM
chandlerc edited edge metadata.

I think you should just fix the code. This seems clearly a bug.

With that fix, this LGTM.

lib/Driver/ToolChains.cpp
3096 ↗(On Diff #28121)

This is certainly wrong. Please just break here, that is what the code should have been doing. We don't want to synthesize a LE multiarch location for a non-LE triple ever.

(Also, good catch!)

This revision is now accepted and ready to land.Jun 26 2015, 1:09 AM
This revision was automatically updated to reflect the committed changes.