[ELF] Improve error message for incompatible section flags

[arichardson]

Summary

Previously we were not printing out the flags of the incompatible section which made it difficult to determine what the problem was.

The error message format has been change to the following:

error: incompatible section flags for .bar
>>> /foo/bar/incompatible-section-flags.s.tmp.o:(.bar): 0x403
>>> output section .bar: 0x3

[grimar]

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

[grimar]

Ah sorry, Sec is an output section here. My suggestion will not work :) Need to think more about it.

[arichardson]

In D32484#736684, @grimar wrote:

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

I think that output format looks better but I don't see how we can determine whichinput file the OutputSection flags are derived from as they are a bitwise or of multiple files.
I guess one option is adding list of InputSections that caused the Flags field to be updated (which would be empty if Sec->Flags == Sec->Sections[0]->Flags)?

Also with very long paths it might make sense to have the flags printed first?

[grimar]

In D32484#736693, @arichardson wrote:

In D32484#736684, @grimar wrote:

It seems both old and new way will not show proper location if section is coming from achieve object ?
We have std::string lld::toString(const InputFile *F) that can return "foo.a(bar.o)" for such case.

Also original implementation and one from this patch does not show where Sec is coming from.
And "for .bar" part probably does not make much sense because its already stated "with others with the same name "

I would do message more explicit, I am thinking about something like next format:

Section .foo has flags incompatible with others with the same name:
>>> defined in foo.a(bar.o) with flags 0x403
>>> defined in zed.o with flags 0x3

Rui, what do you think ?

I think that output format looks better but I don't see how we can determine whichinput file the OutputSection flags are derived from as they are a bitwise or of multiple files.
I guess one option is adding list of InputSections that caused the Flags field to be updated (which would be empty if Sec->Flags == Sec->Sections[0]->Flags)?

Also with very long paths it might make sense to have the flags printed first?

Yeah, I posted suggestion too early, sorry, it will not work. And we also can't assume that sections has the same names,
I think this function used for script as well. It probably can be something like:

Section .input_section_name has flags (0x403) incompatible with output section .output_section_name flags (0x3).
>> .input_section_name is defined in foo.a(bar.o)

[arichardson]

changed error message

[arichardson]

Add full stop at end of message.

[ruiu]

ELF/OutputSections.cpp

411

Error message usually should start with a lowercase letter and shouldn't end with a period. Reason being that it allows you to construct new error messages from other error messages. Say, when you have a string "section does not exist: foo", then you can construct a new error message by prepending another message, e.g. "error while reading file: section does not exist: foo". (We do not construct error messages from other error messages at the moment, though.)

[arichardson]

Start with lowercase letter and remove full stop.

[ruiu]

ELF/OutputSections.cpp

407

Remove TODO. This is a nice improvement but not mandatory.

409–412

Do you think this message is in line with other error messages? For example, we have this error message for TLS attribute mismatch:

error: TLS attribute mismatch: foo
>>> defined in xyz.o
>>> defined in abc.o

I'd probably do something like this.

error: section attribute mismatch
>>> abc.o:(.foo): 0x12
>>> xyz.o:(.foo): 0x34

[arichardson]

Make error message more like other error messages

[ruiu]

LGTM

[arichardson]

Could you commit for me? I don't have commit access.

ELF/OutputSections.cpp

if (!IS->Live) {
     reportDiscarded(IS);
     return;
   }
 
   SectionKey Key = createKey(IS, OutsecName);
   uint64_t Flags = getOutFlags(IS);
   OutputSection *&Sec = Map[Key];
   if (Sec) {
+    // TODO: pretty print the section flags
     if (getIncompatibleFlags(Sec->Flags) != getIncompatibleFlags(IS->Flags))
-      error("Section has flags incompatible with others with the same name " +
-            toString(IS));
+      error("Section " + IS->Name + " flags (0x" +  utohexstr(IS->Flags) +
+            ") are incompatible with output section " + Sec->Name +
+            " flags (0x" + utohexstr(Sec->Flags) + ")\n>>> input section " +
+            IS->Name + " is defined in " + toString(IS->File));
     if (Sec->Type != IS->Type) {
       if (canMergeToProgbits(Sec->Type) && canMergeToProgbits(IS->Type))
         Sec->Type = SHT_PROGBITS;
       else
         error("Section has different type from others with the same name " +
               toString(IS));
     }
     Sec->Flags |= Flags;

test/ELF/incompatible-section-flags.s

 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
 // RUN: not ld.lld -shared %t.o -o %t 2>&1 | FileCheck %s
 
-// CHECK: error:  Section has flags incompatible with others with the same name {{.*}}incompatible-section-flags.s.tmp.o:(.foo)
-// CHECK: error:  Section has flags incompatible with others with the same name {{.*}}incompatible-section-flags.s.tmp.o:(.bar)
+// CHECK:      error: Section .foo flags (0x3) are incompatible with output section .foo flags (0x403)
+// CHECK-NEXT: >>> input section .foo is defined in {{.*}}incompatible-section-flags.s.tmp.o
 
+// CHECK:      error: Section .bar flags (0x403) are incompatible with output section .bar flags (0x3)
+// CHECK-NEXT: >>> input section .bar is defined in {{.*}}incompatible-section-flags.s.tmp.o
 .section .foo, "awT", @progbits, unique, 1
 .quad 0
 
 .section .foo, "aw", @progbits, unique, 2
 .quad 0
 
 
 .section .bar, "aw", @progbits, unique, 3
 .quad 0
 
 .section .bar, "awT", @progbits, unique, 4
 .quad 0