[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 added a comment.

In D72194#1965731 <https://reviews.llvm.org/D72194#1965731>, @MaskRay wrote:

> I am mostly fine with the LLVM CodeGen/MC side change, but the clang side change can probably be moved to a subsequent change. For the `test/clang/CodeGen/` test, we may want to test .ll -> .s instead, not .c -> .s


I didn't understand the terminology here. I presume "interdiff" refers to this diff now having clang and llvm changes in one diff? Is this a problem? I was under the impression that with the mono-repository now cross-project diffs are ok? To be clear I don't have a problem moving the clang error handling to another review if that's not correct/desirable. I just don't understand the benefit.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:743
+        (Section->getEntrySize() != getEntrySizeForKind(Kind)))
+      report_fatal_error(
+          "Symbol '" + GO->getName() + "' from module '" +
----------------
bd1976llvm wrote:
> efriedma wrote:
> > bd1976llvm wrote:
> > > 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!
> > The reason the infrastructure here is, and has remained, sort of shaky, is that we generate very few diagnostics this way in practice.  For IR generated by clang, outside of inline asm, it's almost always a frontend bug if we trigger an error in the backend.
> > 
> > If the backend triggers an assembler diagnostic outside of inline asm, that's 100% a bug.  We should not be generating invalid assembly; that would imply the backend doesn't understand the code its generating.  But TargetLoweringObjectFileELF is part of LLVM codegen, not MC, despite the fact that that it works almost exclusively with MC objects.  So we shouldn't consider this an "assembler" diagnostic.
> > 
> > Generally, the reason people tend to use report_fatal_error is that it's easy: you don't have to worry about formatting the diagnostic, or the implications of what happens after the error is triggered, or a location for the diagnostic.  But really, we want to avoid that if possible.  The documentation here could be improved, though.
> @efriedma - really appreciate you taking the time to explain this. I have run out of time to update this patch this week. On Monday I will look into using LLVMContext::diagnose as you suggested. Thanks!
I have tried to plum in the minimum to report errors using LLVM and Clangs diagnostic stuff. Please take a look.


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

https://reviews.llvm.org/D72194