[PATCH] D30724: Dont emit Mapping symbols for sections that contain only data.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

Sorry about the delay, I got overtaken by events.

It's probably a good idea to make the same change to AArch64, if only to avoid divergence between the two backends.



================
Comment at: lib/MC/MCObjectStreamer.cpp:174
 
+void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) {
+  MCStreamer::EmitLabel(Symbol, Loc);
----------------
Why doesn't this use F?


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:535-536
   void EmitBytes(StringRef Data) override {
+    DelayEmitDataMappingSymbol();
     EmitDataMappingSymbol();
     MCELFStreamer::EmitBytes(Data);
----------------
I'm not convinced splitting these two functions up makes much sense. They interact in really weird ways based on the LastEMS properties.

I think it would be clearer to have a single function that delays  if LastEMS is EMS_None.

    void EmitDataMappingSymbol() {
      if (LastEMS == EMS_Data)
        return;
      else if (LastEMS == EMS_None) {
        // This is a tentative symbol, it won't really be emitted until it's actually needed.
        ElfMappingSymbolInfo *EMS = LastEMSInfo.get();
        EMS->Loc = SMLoc();
        EMS->F = getCurrentFragment();
        auto *DF = dyn_cast_or_null<MCDataFragment>(EMS->F);
        if (DF)
          EMS->Offset = DF->getContents().size();
        LastEMS = EMS_Data;
        return;
      }
      EmitMappingSymbol("$d");
      LastEMS = EMS_Data;
    }


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:540
 
+  void EmitDataMappingSymbolForAnySkipped() {
+    if (!LastEMSInfo->hasInfo())
----------------
FlushPendingMappingSymbol instead?


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:630-631
+    // executable permissions.
+    if (LastEMS != EMS_None ||
+        (cast<MCSectionELF>(getCurrentSectionOnly()))->UseCodeAlign()) {
+      LastEMSInfo->resetInfo();
----------------
I still don't see why we need this `UseCodeAlign` special case, I'm afraid.


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:701
+  DenseMap<const MCSection *,
+           std::pair<ElfMappingSymbol, std::unique_ptr<ElfMappingSymbolInfo>>>
+      LastMappingSymbols;
----------------
I think you'd just as well put the ElfMappingSymbol into the ElfMappingSymbolInfo at this point, and make it clear that it's the whole mapping-symbol state for a given section.


https://reviews.llvm.org/D30724