Page MenuHomePhabricator

Basic block sections for functions with implicit-section-name attribute
ClosedPublic

Authored by tmsriram on Apr 26 2021, 10:51 AM.

Details

Summary

Functions can have section names set via #pragma or section attributes, basic block sections should be correctly named for such functions.

With #pragma, the expectation is that all functions in that file are placed in the same section in the final binary. Basic block sections should be correctly named with the unique flag set so that the final binary has all the
basic blocks of the function in that named section. This patch fixes the bug by calling getExplictSectionGlobal when implicit-section-name attribute is set to make sure the function's basic blocks get the correct section name.

Diff Detail

Event Timeline

tmsriram created this revision.Apr 26 2021, 10:51 AM
tmsriram requested review of this revision.Apr 26 2021, 10:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 10:51 AM
rahmanl added inline comments.Apr 26 2021, 8:06 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
752–759

Except may for chromium with its complex implementation detail here: https://chromium.googlesource.com/chromium/src/+/66.0.3359.158/base/memory/protected_memory.h
Any thoughts on whether this would work with it (related bug: https://bugs.chromium.org/p/chromium/issues/detail?id=990942).

880

It seems like only the last parameter is changed between this call and the one above. Maybe it is OK to change the original function's signature?

tmsriram marked an inline comment as done.Apr 26 2021, 8:53 PM
tmsriram added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
752–759

Section attributes work just fine with the patch. I can add another test. This "," separated specific usage however does not seem to do anything with linux ELF. The "," is just treated as another character.

LGTM with minor comments.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
752–758

This sentence sounds more relevant to the callsite of the function. Here maybe we can simply say "Get a unique ID if forced to emit unique sections. [the rest of the comment]"

752–759

It's probably fine because we only do unique sections for code (and only with the -basic-block-sections option) and the section flags are always the same 'ax'. Anyhow, we can look at this when we build chromium with basic block sections.

llvm/test/CodeGen/X86/basic-block-sections-pragma-sections.ll
6

nit: This is not really ordering basic block sections. Maybe "list" is a better name?

tmsriram updated this revision to Diff 341063.Apr 27 2021, 7:54 PM
tmsriram marked 3 inline comments as done.

Address reviewer comments:

  1. Add another test for section name
  2. Fix nits.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
880

I looked at all the call sites for this function and this signature is used across MachO, COFF, etc. Futher, all other functions that get sections use the same method. One thing I could do is to use default parameters. I am keeping it unchanged but I can modify it if you like.

snehasish accepted this revision.Apr 28 2021, 12:03 PM

lgtm

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
657

nit: If you use a reference here then you can remove the deref and simplify the diff.

789

nit: /* ForceUnique = */ false

This revision is now accepted and ready to land.Apr 28 2021, 12:03 PM
This revision was landed with ongoing or failed builds.Apr 29 2021, 12:30 PM
This revision was automatically updated to reflect the committed changes.