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

[ben via Phabricator via llvm-commits]

bd1976llvm marked an inline comment as done.
bd1976llvm added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:743
+        (Section->getEntrySize() != getEntrySizeForKind(Kind)))
+      report_fatal_error(
+          "Symbol '" + GO->getName() + "' from module '" +
----------------
efriedma wrote:
> 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.
Hi @efriedma, thanks for suggesting this. Sorry I couldn't look into it yesterday - I can take a look today (uk time).

I don't have a good handle on LLVM diagnostics. From poking/looking around I got the impression that in LLVM report_fatal_error was commonly used and that recently https://reviews.llvm.org/rG8cedf0e2994c1a258902ed931abdec5f94725a55 changed the behaviour so that now this aborts. I did see that MCContext::reportError/Warning was only setup when assembling; however, I was under the impression that this should be "the" way to report errors (from MC at least) and that it not being setup to do so outside of assembling was an oversight that hasn't be recognized until now as people were happy with report_fatal_error (before the recent behaviour change).

Do you have any code pointers/insight into the intended design here or thoughts on how LLVM should be moving w.r.t diagnostics .

Is there anyone else who would be good to bring into this?

Any input would be great. Thanks!


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

https://reviews.llvm.org/D72194