This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][ObjC] Remove the leading 'l_' from ObjC symbols and make private symbols in the __DATA segment internal.
ClosedPublic

Authored by ahatanak on May 2 2019, 10:55 AM.

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.May 2 2019, 10:55 AM
rjmccall added inline comments.May 2 2019, 11:20 AM
lib/CodeGen/CGObjCMac.cpp
1024

Please document what this parameter means.

2000
  1. Why are these using different classes to name these enumerators?
  2. Why are we promoting to internal linkage in some cases? That seems to be an additional semantic change not discussed in the commit message, and it should probably also get a good inline comment.
vsk added a comment.May 2 2019, 11:33 AM

Thanks for working on this!

lib/CodeGen/CGObjCMac.cpp
2005

Are constant NSStrings within the shared cache ever dirtied? I was under the impression that they couldn't be, so I'm not sure there's an advantage to making them internal.

3132

Could you use '/*ExplicitDataSegment=*/false' (or similar) for clarity here and elsewhere?

ahatanak updated this revision to Diff 197877.May 2 2019, 3:21 PM
ahatanak marked 6 inline comments as done.
ahatanak edited the summary of this revision. (Show Details)

Address review comments.

ahatanak added inline comments.May 2 2019, 3:22 PM
lib/CodeGen/CGObjCMac.cpp
2000

This is a mistake. It should have been llvm::GlobalValue::InternalLinkage.

2005

I was making all symbols in __DATA internal, but you are right, there isn't any advantage to making constant NSStrings internal for the purpose of this patch.

vsk added inline comments.May 2 2019, 3:34 PM
lib/CodeGen/CGObjCMac.cpp
3965

Hm. I wonder whether it'd be less error-prone to simply define LT as 'Section.empty() || Section.startswith("__DATA")'. Is there an advantage to explicitly defining 'ExplicitDataSegment' in the caller?

ahatanak marked an inline comment as done.May 2 2019, 3:40 PM
ahatanak added inline comments.
lib/CodeGen/CGObjCMac.cpp
3965

I was just trying to avoid scanning the string, but probably doing so isn't that expensive.

rjmccall added inline comments.May 2 2019, 3:57 PM
lib/CodeGen/CGObjCMac.cpp
3965

Yeah, probably not. I'd love it if there was some better way to specify these sections that felt less Mach-O-specific and more semantic, but given what we're doing, I think it'd be cleaner to scan the section name.

Or you could just pass the linkage down; I'm not sure DATA vs. OBJC is the only interesting division between things.

ahatanak updated this revision to Diff 197918.May 2 2019, 10:31 PM
ahatanak marked 2 inline comments as done.
  • Instead of passing a flag to CreateMetadataVar that indicates the section is in segment __DATA, just scan the section name string inside CreateMetadataVar.
  • Fix test case ns-constant-strings.m.
compnerd added inline comments.
lib/CodeGen/CGObjCMac.cpp
3979

Hmm, when would you have a metadata variable not in the __DATA segment?

7255

Is there a reason to not make this and the other instances Internal irrespective of the object file format?

ahatanak marked 2 inline comments as done.May 3 2019, 12:46 AM
ahatanak added inline comments.
lib/CodeGen/CGObjCMac.cpp
3979

There are several places where a section string that starts with __OBJC is passed. For example, CGObjCMac::EmitModuleSymbols.

7255

When the object file format isn't MachO, this variable doesn't go into a section that is in __DATA, so we want to keep the variable private to avoid needlessly preserving the symbol name.

The intent of the patch is to prevent the linker from removing the symbol names of symbols in __DATA so that tools can collect information about those symbols.

ahatanak marked an inline comment as done.May 3 2019, 1:05 AM
ahatanak added inline comments.
lib/CodeGen/CGObjCMac.cpp
7255

I realized that there are other places where I should check the object file format so that the linkage isn't changed to internal when it doesn't have to be changed.

ahatanak updated this revision to Diff 198118.May 3 2019, 6:04 PM

Make sure private linkage is replaced with internal linkage only when the object file format is MachO.

vsk added inline comments.May 7 2019, 5:16 PM
lib/CodeGen/CGObjCMac.cpp
6035

I see some minor variant of this logic repeated a few times. Wdyt of defining a 'getLinkageTypeForObjCMetadata(CGM, Section)' helper to consolidate it in one place?

ahatanak updated this revision to Diff 198581.May 7 2019, 8:25 PM
ahatanak marked an inline comment as done.

Define helper function getLinkageTypeForObjCMetadata.

vsk accepted this revision.May 8 2019, 9:23 AM

Lgtm, thanks!

This revision is now accepted and ready to land.May 8 2019, 9:23 AM
This revision was automatically updated to reflect the committed changes.