Page MenuHomePhabricator

[MC] Create unique .pdata sections for every .text section
ClosedPublic

Authored by rnk on Apr 21 2016, 11:00 AM.

Details

Summary

This adds a unique ID to the COFF section uniquing map, similar to the
one we have for ELF. The unique id is not currently exposed via the
assembler because we don't have a use case for it yet. Users generally
create .pdata with the .seh_* family of directives, and the assembler
internally needs to produce .pdata and .xdata sections corresponding to
the code section.

The map to remember which .pdata section to use for a given .text
section is maintained in the MCContext, since this is something the
assembler needs to maintain to implement .seh_* directives. I'm open to
suggestisons for a better home.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 54543.Apr 21 2016, 11:00 AM
rnk retitled this revision from to [MC] Create unique .pdata sections for every .text section.
rnk added reviewers: majnemer, rafael.
rnk updated this object.
rnk added a subscriber: llvm-commits.
rafael edited edge metadata.Apr 22 2016, 2:40 PM

Just a few quick comments for now.

include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
135 ↗(On Diff #54543)

Why start with 1? Not that it is really important, but ELF start with 0.

include/llvm/MC/MCContext.h
91 ↗(On Diff #54543)

Can't this just be a field in MCSectionCOFF like it is for ELF?

410 ↗(On Diff #54543)

Can you move this out of the context? It looks similar to ELF relocation sections. We handle that mapping in the elf writer and we only createELFRelSection in the context.

rnk added a comment.Apr 25 2016, 10:34 AM

Thanks for the input, I'll try moving this to MCWinCOFFStreamer.

include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
135 ↗(On Diff #54543)

What is the generic unique ID for ELF? What unique ID do I get if I write .text?

I assumed it was zero, but if it's ~0U, then sure I can start this at zero.

include/llvm/MC/MCContext.h
91 ↗(On Diff #54543)

This should have more comments. It exists to map from text section to the unique id of its corresponding pdata section, so that if a text section contains more than one function using SEH, we can reuse the same pdata section for it. This matters for C++ funclets, which should also have a test.

410 ↗(On Diff #54543)

IIUC, ELF relocation sections are below the abstraction level of the assembler. They're something created automatically based on your symbol references.

While we could push .pdata down to that level, the .xdata section is not below that level. The user needs to be able to put their LSDA there. It is a real section opened up by .seh_handlerdata that can be pushed and popped.

I could maintain this mapping in MCWinCOFFStreamer, though, and that would be close to what you're thinking of?

Another twist is that lib/CodeGen/AsmPrinter/WinException.cpp needs to switch to the .xdata section. On 32-bit x86, it doesn't use the .seh_handlerdata directive to do it. Should I have getXDataSectionForTextSection be a method of MCStreamer, or have WinException downcast the MCStreamer to an MCWinCOFFStreamer?

rnk updated this revision to Diff 54959.Apr 25 2016, 6:12 PM
rnk edited edge metadata.
  • Remove DenseMap in favor of MCSectionCOFF field
rnk updated this revision to Diff 54960.Apr 25 2016, 6:21 PM

Revert unintentional change

rnk added a comment.Apr 25 2016, 6:24 PM

This addresses some of your concerns, but it still makes MCStreamer aware of some COFF-specific details. Do you think I have to hide them? If so, what's the best way to do that, given that "target neutral" code in lib/CodeGen/AsmPrinter/WinException.cpp and lib/MC/MCAsmStreamer.cpp use this?

Looking again, sorry for the long delay.

rafael added inline comments.Apr 30 2016, 11:49 AM
include/llvm/MC/MCSectionCOFF.h
36 ↗(On Diff #54960)

one-to-many, no?

39 ↗(On Diff #54960)

So, this is not the unique id of this section, it is the id of the related .xdata and .pdata, correct?

Why do you have to implement it like that? For ELF we store the unique id of the section itself. Then when you want to construct a .xdata or .pdata for this .text you can just use the existing unique id, no? So for example, you would end up with 3 section, .text, .xdata and .pdata all of which have an unique id of 42.

Maybe the name unique id is confusing. It is not really unique, it is really just an disambiguator when the rest (name, flags) are the same.

rnk added inline comments.May 2 2016, 10:43 AM
include/llvm/MC/MCSectionCOFF.h
36 ↗(On Diff #54960)

For every object-file level .text section, there should be one .pdata section and one .xdata section. Maybe I'll just spell that out.

39 ↗(On Diff #54960)

But these two .text sections will have the same unique ID (~0U, the generic section id):

.section        .text,"xr",discard,f
.section        .text,"xr",associative,f

And we need a separate .pdata section for each, and they look like:

.section        .pdata,"xr",associative,f
.section        .pdata,"xr",associative,f

However, they are created implicitly when assembling the .seh_* directives.

We could assign all sections with the generic ID a non-generic unique ID on creation. We would insert two entries into the uniquing map: one with the generic ID, and one with the specific ID.

At the end of the day, I feel like it's better to just maintain this WinCFI unique ID in the .text section.

Comment at: include/llvm/MC/MCSectionCOFF.h:36
@@ +35,3 @@
+ / The unique IDs used with the .pdata and .xdata sections created internally
+
/ by the assembler. This ID is used to ensure that there is a one-to-one
+ /// mapping from .text section to .pdata and .xdata sections. This data is


rafael wrote:

one-to-many, no?

For every object-file level .text section, there should be one .pdata section and one .xdata section. Maybe I'll just spell that out.

Yes, please :-)

Maybe the name unique id is confusing. It is not really unique, it is really just an disambiguator when the rest (name, flags) are the same.

But these two .text sections will have the same unique ID (~0U, the generic section id):

.section        .text,"xr",discard,f
.section        .text,"xr",associative,f

And we need a separate .pdata section for each, and they look like:

.section        .pdata,"xr",associative,f
.section        .pdata,"xr",associative,f

However, they are created implicitly when assembling the .seh_* directives.

We could assign all sections with the generic ID a non-generic unique ID on creation. We would insert two entries into the uniquing map: one with the generic ID, and one with the specific ID.

At the end of the day, I feel like it's better to just maintain this WinCFI unique ID in the .text section.

I see. I guess you could also store two MCSectionELF pointers, but
since UniqueID is not user visible it is probably fine to use as is.
Please add the above explanation as a comment.

LGTM with that.

Cheers,
Rafael

This revision was automatically updated to reflect the committed changes.