[llvm-commits] [llvm] r126127 - in /llvm/trunk/lib: CodeGen/TargetLoweringObjectFileImpl.cpp MC/MCSectionMachO.cpp

[Stuart Hastings]

On Mar 17, 2011, at 11:09 AM, Daniel Dunbar wrote:

> Hi Stuart,
> 
> On Mon, Feb 21, 2011 at 9:27 AM, Stuart Hastings <stuart at apple.com> wrote:
>> Author: stuart
>> Date: Mon Feb 21 11:27:17 2011
>> New Revision: 126127
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=126127&view=rev
>> Log:
>> Fix to correctly support attribute((section("__DATA, __common"))).
>> Radar 9012638.
>> 
>> Modified:
>>    llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
>>    llvm/trunk/lib/MC/MCSectionMachO.cpp
>> 
>> Modified: llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp?rev=126127&r1=126126&r2=126127&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp Mon Feb 21 11:27:17 2011
>> @@ -630,7 +630,7 @@
>>                          Mangler *Mang, const TargetMachine &TM) const {
>>   // Parse the section specifier and create it if valid.
>>   StringRef Segment, Section;
>> -  unsigned TAA, StubSize;
>> +  unsigned TAA = (unsigned)MCSectionMachO::SECTION_ATTRIBUTES, StubSize = 0;
>>   std::string ErrorCode =
>>     MCSectionMachO::ParseSectionSpecifier(GV->getSection(), Segment, Section,
>>                                           TAA, StubSize);
>> @@ -643,10 +643,19 @@
>>     return DataSection;
>>   }
>> 
>> +  bool TAAWasSet = (TAA != MCSectionMachO::SECTION_ATTRIBUTES);
>> +  if (!TAAWasSet)
>> +    TAA = 0;      // Sensible default if this is a new section.
>> +
>>   // Get the section.
>>   const MCSectionMachO *S =
>>     getContext().getMachOSection(Segment, Section, TAA, StubSize, Kind);
>> 
>> +  // If TAA wasn't set by ParseSectionSpecifier() above,
>> +  // use the value returned by getMachOSection() as a default.
>> +  if (!TAAWasSet)
>> +    TAA = S->getTypeAndAttributes();
>> +
>>   // Okay, now that we got the section, verify that the TAA & StubSize agree.
>>   // If the user declared multiple globals with different section flags, we need
>>   // to reject it here.
>> 
>> Modified: llvm/trunk/lib/MC/MCSectionMachO.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCSectionMachO.cpp?rev=126127&r1=126126&r2=126127&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/MC/MCSectionMachO.cpp (original)
>> +++ llvm/trunk/lib/MC/MCSectionMachO.cpp Mon Feb 21 11:27:17 2011
>> @@ -101,16 +101,16 @@
>>     return;
>>   }
>> 
>> -  OS << ',';
>> -
>>   unsigned SectionType = TAA & MCSectionMachO::SECTION_TYPE;
>>   assert(SectionType <= MCSectionMachO::LAST_KNOWN_SECTION_TYPE &&
>>          "Invalid SectionType specified!");
>> 
>> -  if (SectionTypeDescriptors[SectionType].AssemblerName)
>> +  if (SectionTypeDescriptors[SectionType].AssemblerName) {
>> +    OS << ',';
>>     OS << SectionTypeDescriptors[SectionType].AssemblerName;
>> -  else
>> -    OS << "<<" << SectionTypeDescriptors[SectionType].EnumName << ">>";
>> +  } else
>> +    // If we have no name for the attribute, stop here.
>> +    return;
>> 
>>   // If we don't have any attributes, we're done.
>>   unsigned SectionAttrs = TAA & MCSectionMachO::SECTION_ATTRIBUTES;
>> @@ -125,7 +125,9 @@
>> 
>>   // Check each attribute to see if we have it.
>>   char Separator = ',';
>> -  for (unsigned i = 0; SectionAttrDescriptors[i].AttrFlag; ++i) {
>> +  for (unsigned i = 0;
>> +       SectionAttrs != 0 && SectionAttrDescriptors[i].AttrFlag;
>> +       ++i) {
>>     // Check to see if we have this attribute.
>>     if ((SectionAttrDescriptors[i].AttrFlag & SectionAttrs) == 0)
>>       continue;
>> @@ -207,7 +209,6 @@
>>            "between 1 and 16 characters";
>> 
>>   // If there is no comma after the section, we're done.
>> -  TAA = 0;
> 
> It looks like you changed the semantics of ParseSectionSpecifier, but
> didn't update all callers. The side effect is that this ends up
> allowing MC to emit sections with bogus flags.

Sorry about that.

> I added a minimal fix in r127812, but I find the code in
> TargetLoweringObjectFileImpl a bit gross. Is it just using
> MCSectionMachO::SECTION_ATTRIBUTES as a sentinel value?

Yes.

> I think it
> would be better to have ParseSectionSpecifier explicitly return a bool
> indicating whether the TAA was parsed or not, instead of using a
> sentinel.

I was reluctant to add a sixth parameter to ParseSectionSpecifier, but if you think that's better, I'll do that.

stuart

> Or at a minimum, it would be good to comment what
> TargetLoweringObjectFileMachO::getExplicitSectionGlobal is doing.
> 
> Thanks,
> - Daniel
> 
>>   StubSize = 0;
>>   if (Comma.second.empty())
>>     return "";
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>