Page MenuHomePhabricator

Do not construct std::string from nullptr
ClosedPublic

Authored by georgthegreat on Sep 15 2020, 7:49 AM.

Details

Summary

While I am trying to forbid such usages systematically in https://reviews.llvm.org/D79427 / P2166R0 to C++ standard, this PR fixes this (definitelly incorrect) usage in llvm.

This code is unreachable, so it could not cause any harm

Diff Detail

Event Timeline

georgthegreat created this revision.Sep 15 2020, 7:49 AM
georgthegreat requested review of this revision.Sep 15 2020, 7:49 AM
nikic accepted this revision.Sep 25 2020, 1:02 PM

LGTM

This revision is now accepted and ready to land.Sep 25 2020, 1:02 PM

I would rather see:

llvm_unreachable("unexpected type");
return ""; // or string()

i.e, keep the call to unreachable.

nikic added inline comments.Sep 25 2020, 2:09 PM
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1277

@mclow.lists There already is a call to llvm_unreachable here ^ so the second call to unreachable seems redundant.

mclow.lists added inline comments.Sep 25 2020, 2:23 PM
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1277

I know there are compilers that will warn if you get to the end of a fn and there's no return statement there. Don't know how good clang is about analyzing all the paths through a function and determining that all of them lead to a return (and/or unreachable)

To make it easy on the compiler (and the reader), if I was writing this, I would move the default case to the end, and make the function look end look like this:

    else
      return "u32";
  default:
      break;
  }
  llvm_unreachable("unexpected type");
  return "";
}
1299

But I would also rewrite this chunk as:

if (static_cast<const NVPTXTargetMachine &>(TM).is64Bit())
    return useB4PTR ? "b64" : "u64";
else
    return useB4PTR ? "b32" : "u32";

But now I'm indulging in feature-creep :-) so you can ignore this.

georgthegreat added inline comments.Sep 26 2020, 2:57 AM
llvm/lib/Target/NVPTX/NVPTXAsmPrinter.cpp
1277

This patch has been integrated into our monorepo and compiles just find with at least clang7 and clang10.

As all the branches of the switch above result in either return of llvm_unreachable invocation, I have considered the extra llvm_unreachable call redundant.

I do not have any string preferences here. @mclow.lists, what would you suggest after all the considerations?

1299

This is not the code I am familiar with, so I just wanted to fix the obvious compilation / assertion.

Restore unreachable return statement, upon mclow.lists@ request.

So, could someone, please, commit this PR? I have no write access to llvm.

I would rather see:

llvm_unreachable("unexpected type");
return ""; // or string()

i.e, keep the call to unreachable.

I'd split the difference and keep the unreachable but drop the return - llvm_unreachable is annotated noreturn and all the compilers we use for LLVM are OK with/don't warn on: "non_void f1() { llvm_unreachable(...); }"

As for the possibility of relying on a switch with a default, where all cases and the default return or call a no-return function. I'd be OK with that, if it works for all the compilers LLVM supports - if there's prior art/examples of this in the LLVM codebase, that'd be adequate for me.

& yeah, I'd be OK with @mclow.lists 's suggestion to move the default to the end and just have it break, keeping a singular unreachable (& though I'd favor fairly strongly not having a return after that) outside the switch.

Changed as dblaikie@ suggested. Not all the types are handled during the switch, so default is still required in order to prevent compiler from issuing warnings.

dblaikie accepted this revision.Nov 5 2020, 11:32 AM

Good for me - please wait for @mclow.lists to weigh in/close the loop as well, though.

I'm fine with this.
This is not how I'd write it, but that's ok :-)

@dblaikie, so, could you, please, commit this at last?

@dblaikie, so, could you, please, commit this at last?

Oh, sure thing! Should be committed shortly.

This revision was automatically updated to reflect the committed changes.