[PATCH] D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:743
+        (Section->getEntrySize() != getEntrySizeForKind(Kind)))
+      report_fatal_error(
+          "Symbol '" + GO->getName() + "' from module '" +
----------------
bd1976llvm wrote:
> bd1976llvm wrote:
> > MaskRay wrote:
> > > nickdesaulniers wrote:
> > > > If we have a way of producing a non-fatal warning, that may be preferred to crashing the compiler, which encourages users to send us a bug report. In this case, we don't care about their buggy code with impossible to solve constraints, and the text you provide here should be enough information for them to solve their code.
> > > > 
> > > > Maybe a warning, but proceed with codegen? The warning can encourage them that referencing those variables is undefined behavior.
> > > This code path is possible, which means we should use MCContext::reportError rather than report_fatal_error. In a bugfree world, report_fatal_error should not ever be called.
> > report_fatal_error is used in similar circumstances in this file (which is why I chose it). See for example:
> > 
> > ```
> > TargetLoweringObjectFileMachO::getExplicitSectionGlobal(
> > ...
> >   if (!ErrorCode.empty()) {
> >     // If invalid, report the error with report_fatal_error.
> >     report_fatal_error("Global variable '" + GO->getName() +
> >                        "' has an invalid section specifier '" +
> >                        GO->getSection() + "': " + ErrorCode + ".");
> > 
> > ```
> > 
> > In this file there are no current uses of reportError everything uses report_fatal_error so I wonder if what report_fatal_error -> MCContext::reportError  is a more general refactor that should be done in a separate change?
> > 
> > I tried using MCContext::reportError but it didn't produce any output because llc does not seem to setup a SrcMgr correctly to allow for warning and errors. I have been unable to determine where to modify llc to setup a SrcMgr correctly today. I will look at this problem again tomorrow.
> I think a warning is appropriate as the codegen is correct if the linker does not optimise the mergeable sections. Unfortunately, I can't get a warning to be emitted because llc does not seem to setup a SrcMgr correctly to allow for warning and errors. I have been unable to determine where to modify llc to setup a SrcMgr correctly. I will look at this problem again tomorrow.
MCContext::reportError is intended for assembler errors.  In CodeGen, you probably want to be calling LLVMContext::diagnose.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72194/new/

https://reviews.llvm.org/D72194