This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix a crash in FEntryInserter Pass.
ClosedPublic

Authored by manojgupta on Jul 28 2017, 12:34 AM.

Details

Summary

FEntryInserter pass unconditionally derefs the first Instruction
in the first Basic Block. The pass crashes when the first
BasicBlock is empty. Fix the crash by not dereferencing the basic
Block iterator. This fixes an issue observed when building Linux kernel
4.4 with clang.

Fixes PR33971.

Event Timeline

manojgupta created this revision.Jul 28 2017, 12:34 AM
davide added a subscriber: davide.Jul 28 2017, 12:43 AM

Nice! What's the status of Linux 4.4 + clang?

lib/CodeGen/FEntryInserter.cpp
45

If the first BB is not empty, why you're not still passing `FirstMI.getDebugLoc()?

test/CodeGen/X86/fentry-insertion.ll
15–20

This comment is not needed.

28

Ditto.

manojgupta removed a reviewer: dblaikie.

Linux kernel is still WIP.

Added back the DebugLoc from first instruction. I am not sure though if reusing
debug location from an unrelated instruction is ok.

manojgupta marked 3 inline comments as done.Jul 28 2017, 11:20 AM

David, your comment didn't make it here. So adding it myself.

The call to fentry is unconditionally added to the beginning of function. Should the call use the same debug location of the previous instruction if present?

On Fri, Jul 28, 2017 at 10:33 AM, David Blaikie <dblaikie@gmail.com> wrote:

Depends what you mean by "unrelated" - if the new instruction is created because of/only in the presence of the old instruction, yaeh, using its debugloc may be appropriate. (Though if the new instruction may be created in a different basic block - that gets trickier)

Removed getting DebugLoc from previous instruction based on Dave's comments.

From: David Blaikie <dblaikie@gmail.com> Date: Fri, Jul 28, 2017 at 11:38 AM

  • The call to fentry is unconditionally added to the beginning of function.

& the other instruction may not be in the entry block?

  • Should the call use the same debug location of the previous instruction if present?

No - it'll probably get that location anyway, later on/at code generation time, but you shouldn't add it to the instruction itself if it doesn't have any reason to be related/at that location.

  • Dave

Is it ok to land this?

niravd accepted this revision.Aug 1 2017, 8:33 AM

LGTM.

This revision is now accepted and ready to land.Aug 1 2017, 8:33 AM
manojgupta closed this revision.Aug 1 2017, 8:40 AM
This revision was automatically updated to reflect the committed changes.