Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[LLD][COFF] Handle 'label' symbols when they point to a COMDAT section
AbandonedPublic

Authored by aganea on Aug 4 2023, 1:43 PM.

Details

Summary

This fixes an issue where OBJs files containing a malformed .text$x section references label symbols (that is IMAGE_SYM_CLASS_LABEL) that live in a COMDAT .text$mn. This kind of OBJ files are generated when using binutils objcopy. A tentative reproducer can be found here in https://github.com/llvm/llvm-project/issues/62182. This patch already contains a test that covers the bug.

Fixes PR62182.

Diff Detail

Event Timeline

aganea created this revision.Aug 4 2023, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 1:43 PM
aganea requested review of this revision.Aug 4 2023, 1:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 1:43 PM
compnerd added inline comments.Aug 5 2023, 6:18 PM
lld/COFF/InputFiles.cpp
628

Interesting. I think that we should add a comment about this impacts identity and comparison. Does this change in definition have any impact on ICF? IIRC, we did have a minor difference between link and lld in that regard.

lld/COFF/InputFiles.h
267

Can we use a typedef for the std::vector<std::pair<DefinedRegular *, const llvm::object::coff_aux_section_definition *>>?

rnk added a comment.Aug 10 2023, 11:27 AM

Here are some comments, I have to run, so it's not complete, but hopefully still useful.

lld/COFF/InputFiles.cpp
388

This array is sparse, so we are adding a pointer for every input section, so this could be a performance sensitive change.

645

For my understanding, the problem is that the old code just returned the leader in this case, right? And you have solved the problem by synthesizing a new symbol to represent the label.

lld/test/COFF/Inputs/comdat-malformed-assoc-a.yaml
55

I see, .text$x is not comdat but .text$mn is. I think this test would be more readable in assembly. If you start with assembly output from clang, can you adjust the section directives to create the analogous situation?

Sorry I will answer to your comments a bit later. I was trying to understand the root issue, and I've managed to reproduce it locally.

The problem is not MSVC, but the way the packaging is done in firebase-sdk. Namely, it uses binutils objcopy which doesn't seem to understand COMDAT relationships (that is, IMAGE_COMDAT_SELECT_ASSOCIATIVE). Simply replacing by llvm-objcopy fixes the issue.

I'm also wondering if instead of this patch, we couldn't craft something simpler, an heuristic that "fixes" the section header and the section symbol. Usually they seems to come in a pair, .text$mn, .text$x or a trio if debug symbols are involved: .text$mn, .debug$S, .text$x. Opinions?

aganea edited the summary of this revision. (Show Details)Aug 13 2023, 8:45 PM

I'm now less inclined to change the behaviour of lld to support this. It seems that given that this is a bug in objcopy, it should be something that could be mitigated by using llvm-objcopy, and fixing binutils to correctly comdat would be more useful. Adding additional context in the error message about the missing entries would help identify what is missing and hopefully allow the user to investigate and fix the issue? I also suspect that this is not going to be reproducible via MSVC alone (which was the original assumption), and there are other complications from using binutils to handle PE/COFF files in my experience.

rnk added a comment.Aug 29 2023, 2:59 PM

I'm also wondering if instead of this patch, we couldn't craft something simpler, an heuristic that "fixes" the section header and the section symbol. Usually they seems to come in a pair, .text$mn, .text$x or a trio if debug symbols are involved: .text$mn, .debug$S, .text$x. Opinions?

I think there is prior art for doing this for mingw, we do some convoluted stuff to associate .text$_Z3foov / .pdata$_Z3foov / .xdata$_Z3foov together, but IIRC it's not good for performance.

I think for the moment I'm inclined to do nothing, unless this is really high priority for some user. It sounds like we can recommend using llvm-objcopy in place of binutils objcopy as a possible solution.

aganea added a comment.EditedAug 29 2023, 3:26 PM

I'm also wondering if instead of this patch, we couldn't craft something simpler, an heuristic that "fixes" the section header and the section symbol. Usually they seems to come in a pair, .text$mn, .text$x or a trio if debug symbols are involved: .text$mn, .debug$S, .text$x. Opinions?

I think there is prior art for doing this for mingw, we do some convoluted stuff to associate .text$_Z3foov / .pdata$_Z3foov / .xdata$_Z3foov together, but IIRC it's not good for performance.

I think for the moment I'm inclined to do nothing, unless this is really high priority for some user. It sounds like we can recommend using llvm-objcopy in place of binutils objcopy as a possible solution.

I agree. One argument would be that link.exe supports this scenario without a warning. But since it only came up for a single user, and their root issue will be fixed eventually, it’s not worth adding a fix. We could however add an extra note when that error occurs, and suggest llvm-objcopy as a possible fix, do you think it’s worth it?

rnk added a comment.Aug 29 2023, 4:19 PM

I think this kind of error ("relocation against symbol in discarded section") is really hard to debug, and probably deserves a whole page of documentation. I think the best option would be to emit a short link to online documentation describing the issue category in detail, something like https://lld.llvm.org/missingkeyfunction.html .

aganea abandoned this revision.Sep 15 2023, 1:55 PM