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

[Shankar Easwaran via Phabricator via llvm-commits]

shankare added inline comments.


================
Comment at: lib/MC/MCObjectStreamer.cpp:174
 
+void MCObjectStreamer::EmitLabel(MCSymbol *Symbol, SMLoc Loc, MCFragment *F) {
+  MCStreamer::EmitLabel(Symbol, Loc);
----------------
t.p.northover wrote:
> Why doesn't this use F?
This is a good catch. Thanks!


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:535-536
   void EmitBytes(StringRef Data) override {
+    DelayEmitDataMappingSymbol();
     EmitDataMappingSymbol();
     MCELFStreamer::EmitBytes(Data);
----------------
t.p.northover wrote:
> 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;
>     }
Sure. 


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:540
 
+  void EmitDataMappingSymbolForAnySkipped() {
+    if (!LastEMSInfo->hasInfo())
----------------
t.p.northover wrote:
> FlushPendingMappingSymbol instead?
Sure, I will do that. 


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:630-631
+    // executable permissions.
+    if (LastEMS != EMS_None ||
+        (cast<MCSectionELF>(getCurrentSectionOnly()))->UseCodeAlign()) {
+      LastEMSInfo->resetInfo();
----------------
t.p.northover wrote:
> I still don't see why we need this `UseCodeAlign` special case, I'm afraid.
The only time that we want to emit a Data symbol is if the section was set to be executable but it contains data. 




================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:701
+  DenseMap<const MCSection *,
+           std::pair<ElfMappingSymbol, std::unique_ptr<ElfMappingSymbolInfo>>>
+      LastMappingSymbols;
----------------
t.p.northover wrote:
> 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.
Ok.


https://reviews.llvm.org/D30724