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

[Daniel Dunbar]

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.

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? 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. 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
>