support xcoff for llvm-nm

[DiggerLin]

Summary

add the xcoff symbol type functionality for llvm-nm.

[jhenderson]

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test

1 ↗

(On Diff #381975)

;; (actually probably ## and # for comments for YAML support). Also, should it be "XCOFF" in this comment?

Also, rename the test file to remove the "xcoff" and "nm" from the name. Both are implied by the file path. Given my out-of-line comment suggesting this patch be stripped back to just regular printing, with no options etc, I'd suggest naming it basic.test.

2 ↗

(On Diff #381975)

Can't we use yaml2obj here now? That would allow for much better targeted testing.

llvm/tools/llvm-nm/llvm-nm.cpp

941–944

I know that other formats throw away errors, but I would suggest that's a mistake - how is a user supposed to know why a given symbol results in ?? I'd recommend reporting the Error as a warning, rather than just throwing it away.

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test

2 ↗

(On Diff #381975)

I do not think I can use the yaml2obj now, the yaml2obj do not support "csect Auxiliary Entry for C_EXT, C_WEAKEXT, and C_HIDEXT Symbols" . in the xcoff , some important symbol attributes are stored in the csect Auxiliary Entry" , @esms, if you have time , would you like to consider to implement the functionality into yaml2obj ?

llvm/tools/llvm-nm/llvm-nm.cpp

941–944

I agree with you, But I am prefer to keep the same code style and error handling with other object file format in the patch. and fixing the error handling for all the other object file format in a separate patch.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test

2 ↗

(On Diff #381975)

@Esme

llvm/tools/llvm-nm/llvm-nm.cpp

941–944

You're suggesting the following path:

XCOFF with bad error handling -> Fix handling for all formats (including XCOFF)

I think it would be better to avoid ever having a state where XCOFF has bad handling, so I'd prefer one of the two following:

Land XCOFF with good error handling -> Fix handling of other formats.
Fix handling of other formats -> Land XCOFF with good error handling.

I don't mind if you want to change the other formats up-front, but beware that there is a risk this will be more complicated.

[DiggerLin]

In D112450#3102179, @jhenderson wrote:

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

I agree with you that it may be better to split the patch into three at begin . Since I have implemented all these functionality together and there is not a lot of code change in the "size extraction" and "name demangling" , I do not think we can get a lot of benefit from putting effort to split the patch into three now.

[DiggerLin]

change consumeError to error handle.

[jhenderson]

In D112450#3104128, @DiggerLin wrote:

In D112450#3102179, @jhenderson wrote:

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

I agree with you that it may be better to split the patch into three at begin . Since I have implemented all these functionality together and there is not a lot of code change in the "size extraction" and "name demangling" , I do not think we can get a lot of benefit from putting effort to split the patch into three now.

Please split it up. It will allow me to review each part in isolation. It will also allow for finer grained control of the patches, e.g. because there is an issue with the size one and it needs reverting.

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

941–944

As suggested in my original comment, this should be a warning rather than an error: in general, dumping tools should warn and then do what they can to continue. The warning will describe why the symbols are ? labelled, but you'll still be able to see the list of symbols (which may be of some use).

[DiggerLin]

split the patch

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

2

llvm/tools/llvm-nm/llvm-nm.cpp

158

1. Move this function so that it isn't sandwiched by the error functions.
2. I think you can just use WithColor::defaultWarningHandler to replace most of the logic around handleAllErrors below.
3. For consistency with the error functions in this file, I'd just call this function warn rather than reportWarning.

163–164

I think this comment is better (please reflow with clang-format as appropriate).

926–927

You need to handle the case where TypeOrErr is in an error state after this function. Returning '?' is probably reasonable.

You need a test case that shows this warning is printed too.

929

This is the more common way of getting the value from an Expected, at least in places I usually read code. I think it's a little clearer too.

938

Delete this blank line.

939–940

As above - you need to return '?' or similar, to prevent trying to dereference an Expected in a bad state.

1698

This change is unrelated - please revert it.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

158

if using WithColor::defaultWarningHandler(Error Warning) directly , we can not tell user which object file has the warning.

[DiggerLin]

address comment and add a new test cases

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

131

warn not warning - warn is a verb, warning is a noun, and functions should be verbs or verb phrases.

158

That's not true: just pass in an error created via createFileError, right? The implementation of the defaultWarningHandler is almost identical to your existing code.

927

Test case?

941

If possible, add additional context to this error: which symbol has the invalid section?

Something like the following message would be ideal:

"warning: "a.o": unable to load section for symbol "foo": ..."
or
"warning: "a.o": unable to load section for symbol with index 3: ..."

Same goes above.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

158

the defininiton of

void WithColor::defaultWarningHandler(Error Warning) {
   handleAllErrors(std::move(Warning), [](ErrorInfoBase &Info) {
     WithColor::warning() << Info.message() << '\n';
   });
 }

if I write something like

static void warn(Error Err, StringRef Input) {

assert(Err);
if (Input == "-")
  Input = "<stdin>";

// Flush the standard output so that the warning isn't interleaved with other
// output if stdout and stderr are writing to the same place.
outs().flush();
WithColor::defaultWarningHandler(createFileError(Input, std::move(Err)));

}

the ToolName will be lost.

I think we need ToolName in the warning info,
we have ToolName in the error info in llvm-nm

927

in the invalid-section-index.test

1. NM-NEXT: 00000000 ? .text

941

get symbol name maybe error again.
Expected<StringRef> XCOFFObjectFile::getSymbolName(DataRefImpl Symb) const

in order to display more info on a warning, we have to deal with another error or warning, it do not worth it.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test

4–7

You can just call this %t.o. No need to show that it comes from YAML.

When running FileCheck, you should pass in the file name as a variable name, as per the inline edit, and then use the variable in the name check. Also, you need to add an optional .exe suffix for the llvm-nm tool for Windows support.

llvm/tools/llvm-nm/llvm-nm.cpp

158

Right, fair enough about the tool name (you said file name before, which confused me).

941

llvm-readobj's ELF port at least, has a number of examples of how to unwrap an error to add additional context. Typically it might end up as something like:
"warning: 'a.o': unable to load section for symbol 'x' (index: 1): <error message from lower-level library>"

The symbol name will be handled and reported elsewhere, since it needs printing as part of the output. As such, you could just use consumeError and then use a placeholder string like <?> for the name. Alternatively, don't bother with the symbol name entirely and just print the index ("unable to load section for symbol with index 1: ...").

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

42

Coming back to this list of symbols: there are too many, and I doubt you need all of them for your test cases. I'd suggest the following:

1. Consider using assembly if you can as your input, as this will give you greater control over your symbol names and properties.
2. Only have one symbol for each possible code path through the llvm-nm code. 90% of these symbols look like they're just adding noise, and no value.

[DiggerLin]

address comment

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test

4–7

thanks

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

142–143

As a general note, please remember in English prose (such as warning messages) to have a space before opening parentheses. However, in this particular case, I'd suggest a slight restructuring of the message, which will work regardless of the output of EI.message(). See the inline edit.

This puts the symbol index before the rest of the message. In fact, I'd recommend making it even more generic, and change the signature to static void warn(Error Err, Twine Path, Twine Context = Twine()), with it becoming something like:

WithColor::warning(errs(), ToolName) << Path << ": " << (Context.empty() ? "" : Context + ": ")
    << EI.message() << "\n";

The reason is that this is a generic warning method, not "warnForSymbolError" or similar. If you'd like to add an additional wrapper function that converts a symbol index into a context string, before calling this, that would be fine too. The reasons for the suggested changes:

1. The error functions take a Twine Path for the file name, so change this to be consistent with those.
2. There may be occasions in the future of llvm-nm when we want to print a warning where no additional context is needed, so handle that case too.
3. Use Twine rather than StringRef for the Context string, to allow it to be built up dynamically (note that the error function that takes a Message rather than an Error uses Twine for the Message).

Also, as a reminder, modern LLVM style doesn't have full stops at ends of errors and warnings.

940–941

Some suggested improvements. Please reflow too.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

25–26

Why are you not using check-next here? Are there symbols you are deliberately ignoring in this case?

32

Why's this line here?

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/basic.test

25–26

yes, for the convenience to review. there are some symbols are ignore in the case.

32

thanks . delete it.

llvm/tools/llvm-nm/llvm-nm.cpp

940–941

thanks

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

2

Add to this comment what exact behaviours for XCOFF objects and llvm-nm you are testing, e.g. showing that llvm-nm uses can identify the symbols in such an object file and provide appropriate letter codes for them.

18

I'd recommend adding --match-full-lines to FileCheck, to show there's no garbage after symbol names (or in this particular case, that there's no name at all).

25–26

This then raises the question of why are those symbols in the input?

I'm guessing it's so you can test other test cases without having multiple checked-in objects, but that just highlights one of the problems with canned binaries: you cannot tailor them to the individual test case, if you want to avoid having lots to them. I think this just shows you should focus on helping @Esme get the yaml2obj work finished, above getting wider support for XCOFF in the tools.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

26

I guess this is an XCOFF-ism, but this and the below symbol look like functions to me, from their name?

[DiggerLin]

the parent patch of the patch is https://reviews.llvm.org/D113552

1. using yaml2obj for test case
2. change

static void warn(Error Err, Twine FileName, Twine Context = Twine())
to
static void warn(Error Err, Twine FileName, Twine Context = Twine(), Twine Archive = Twine())

[jhenderson]

This is my last day working before Christmas. If I find time to respond to any updates/comments before the end of my work day, I'll do so, but otherwise, don't expect further responses from me until at least the 4th of January (likely a day or two after that).

llvm/test/tools/llvm-nm/XCOFF/basic.test

3–12

I think you could simplify these slightly by doing the following:

1. Move each individual line down to the symbol you are checking (either in the YAML or the CHECK lines, so that you have something like

## Comment
# CHECK: symbol check line
## Comment
# CHECK: symbol check line

2. No need for "Test:" at the start of the statement.
3. End with a "." like other comments.
4. You can drop the "show type char 'X'" part of each comment, because this is implicit by what the test actually does. The bit that's important is what properties of the symbol are significant.

14

No need for --docnum with only one YAML document in the file.

15

Use the default CHECK, since you only have one test case in this file.

llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test

4–8

1. Don't use --docnum/--check-prefix unnecessarily.
2. {{.*}} is unnecessary at the start of a CHECK line, if not using --match-full-lines.

18

Line this up with the other entries in the block.

llvm/tools/llvm-nm/llvm-nm.cpp

141

Please include a test case that uses an archive, and produces this warning, to show that the archive name is printed.

264–265

You need a test case that uses a 64-bit input, and one that uses a 32-bit input, and then demonstrate how this part of the patch impacts the behaviour.

942–943

I'm not sure I see a test case for this particular line?

957

Ditto.

[DiggerLin]

In D112450#3199309, @jhenderson wrote:

This is my last day working before Christmas. If I find time to respond to any updates/comments before the end of my work day, I'll do so, but otherwise, don't expect further responses from me until at least the 4th of January (likely a day or two after that).

Merry Christmas ! Have a good holiday vacation. @jhenderson

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

141

I rollback to previous version, it do not use archive here, I will add archive name for the warn function in "create export list" patch. and there will be a test case.

264–265

add a test case to the 8 byte address are print out in 64bits object file.

942–943

add a test case on N_DEBUG section to test it.

957

add symbol in .except to test.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/basic.test

94

As stated in my earlier comment, there's no need to state "show type char 'W'" or equivalent in the following comments. Please delete.

llvm/test/tools/llvm-nm/XCOFF/basic_64.test

1 ↗

(On Diff #395479)

[jhenderson]

LGTM!

llvm/test/tools/llvm-nm/XCOFF/basic.test

+## Test llvm-nm for XCOFF object files.
+# RUN: llvm-nm  %p/Inputs/test_gcc.o | FileCheck --check-prefix=NM-SYM %s
+
+## test_gcc.o is generated by the following source compiled with g++ in aix os.   
+##  int v = 0;
+## 
+## __attribute__((visibility ("protected"))) int  vp = 1;
+## __attribute__((visibility ("hidden"))) int  vh = 2;
+## __attribute__((visibility ("default"))) int  vd = 3;
+## __attribute__ ((weak)) __attribute__((visibility ("hidden"))) int vwh = 4;
+## __attribute__ ((weak)) __attribute__((visibility ("protected"))) int vwp = 5;
+## 
+## class C {
+## public:
+##  int c;
+##  C(int v):c(v) {}
+## };
+## 
+## C cc(2);
+## 
+## static int si = 6;
+## 
+## static int func0 () {
+##   return vp+si;
+## }
+## 
+## int func1 (int i) {
+##   return func0() * i;
+## }
+## 
+## __attribute__ ((weak)) __attribute__((visibility ("hidden")))
+## int fwh() {
+##   return si+1;
+## }
+## 
+## __attribute__ ((weak)) __attribute__((visibility ("protected")))
+## int fwp() {
+##   return si+2;
+## }
+
+# NM-SYM:      00000240 T ._GLOBAL__I_65535_0_v
+# NM-SYM-NEXT: 000001e4 t ._GLOBAL__sub_I_v
+# NM-SYM-NEXT: 000000b0 W ._Z3fwhv
+# NM-SYM-NEXT: 000000f4 W ._Z3fwpv
+# NM-SYM-NEXT: 00000138 t ._Z41__static_initialization_and_destruction_0ii
+# NM-SYM-NEXT: 00000050 T ._Z5func1i
+# NM-SYM-NEXT: 00000000 t ._ZL5func0v
+# NM-SYM-NEXT: 00000294 t ._ZN1CC1Ei
+# NM-SYM-NEXT: 00000294 W ._ZN1CC1Ei
+# NM-SYM-NEXT: 000002f0 d .data
+# NM-SYM-NEXT: 00000000 f .file
+# NM-SYM-NEXT: 00000000 t .text
+# NM-SYM-NEXT: 00000378 d TOC
+# NM-SYM-NEXT: 0000036c D _GLOBAL__I_65535_0_v
+# NM-SYM-NEXT: 0000036c d _GLOBAL__I_65535_0_v
+# NM-SYM-NEXT: 00000360 d _GLOBAL__sub_I_v
+# NM-SYM-NEXT: 0000033c W _Z3fwhv
+# NM-SYM-NEXT: 00000348 W _Z3fwpv
+# NM-SYM-NEXT: 00000354 d _Z41__static_initialization_and_destruction_0ii
+# NM-SYM-NEXT: 00000330 d _Z5func1i
+# NM-SYM-NEXT: 00000330 D _Z5func1i
+# NM-SYM-NEXT: 0000037c d _ZL2si
+# NM-SYM-NEXT: 00000324 d _ZL5func0v
+# NM-SYM-NEXT: 00000310 W _ZN1CC1Ei
+# NM-SYM-NEXT: 00000320 d _test.rw_
+# NM-SYM-NEXT: 00000308 D cc
+# NM-SYM-NEXT: 00000380 d cc
+# NM-SYM-NEXT: 000002f0 D v
+# NM-SYM-NEXT: 000002fc D vd
+# NM-SYM-NEXT: 000002f8 D vh
+# NM-SYM-NEXT: 000002f4 D vp
+# NM-SYM-NEXT: 00000378 d vp
+# NM-SYM-NEXT: 00000300 W vwh
+# NM-SYM-NEXT: 00000304 W vwp

llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test

+## Test the behavior of the symbol reference section.
+
+# RUN: yaml2obj --docnum=1 %s -o %t_yamlgen.o
+# RUN: llvm-nm %t_yamlgen.o 2>&1 | FileCheck %s --check-prefix=NM
+
+# NM:      {{.*}}llvm-nm: warning: {{.*}}_yamlgen.o': the section index (4) is invalid
+# NM-NEXT: 00000000 ? .text 
+
+--- !XCOFF
+FileHeader:
+  MagicNumber: 0x1DF
+Sections:
+  - Name: .text
+Symbols:
+  - Name:               .text
+    Value:              0x0
+    SectionIndex: 4
+    Type:               0x0
+    StorageClass:       C_STAT
+    NumberOfAuxEntries: 1

llvm/tools/llvm-nm/llvm-nm.cpp

 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/TapiFile.h"
 #include "llvm/Object/TapiUniversal.h"
 #include "llvm/Object/Wasm.h"
+#include "llvm/Object/XCOFFObjectFile.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"

 
 // Miscellaneous states.
 static bool PrintAddress = true;
 static bool MultipleFiles = false;
 static bool HadError = false;
 
 static StringRef ToolName;
 
+static void warning(Error Err, StringRef Input) {
+  assert(Err);
+  if (Input == "-")
+    Input = "<stdin>";
+
+  // Flush the standard output so that the warning isn't interleaved with other
+  // output if stdout and stderr are writing to the same place.
+  outs().flush();
+  handleAllErrors(
+      createFileError(Input, std::move(Err)), [&](const ErrorInfoBase &EI) {
+        WithColor::warning(errs(), ToolName) << EI.message() << "\n";
+      });
+}
+
 static void error(Twine Message, Twine Path = Twine()) {
   HadError = true;
   WithColor::error(errs(), ToolName) << Path << ": " << Message << "\n";
 }
 
 static bool error(std::error_code EC, Twine Path = Twine()) {
   if (EC) {
     error(EC.message(), Path);
     return true;
   }
   return false;
 }
 
 // This version of error() prints the archive name and member name, for example:
 // "libx.a(foo.o)" after the ToolName before the error message.  It sets
 // HadError but returns allowing the code to move on to other archive members.
 static void error(llvm::Error E, StringRef FileName, const Archive::Child &C,
                   StringRef ArchitectureName = StringRef()) {
   HadError = true;
   WithColor::error(errs(), ToolName) << FileName;
 
   Expected<StringRef> NameOrErr = C.getName();
   // TODO: if we have a error getting the name then it would be nice to print
   // the index of which archive member this is and or its offset in the
   // archive instead of "???" as the name.
   if (!NameOrErr) {
     consumeError(NameOrErr.takeError());
     errs() << "(" << "???" << ")";
return std::make_tuple(A.Name, A.Size, A.Address) <
          std::make_tuple(B.Name, B.Size, B.Address);
 }
 
 static char isSymbolList64Bit(SymbolicFile &Obj) {
   if (auto *IRObj = dyn_cast<IRObjectFile>(&Obj))
     return Triple(IRObj->getTargetTriple()).isArch64Bit();
   if (isa<COFFObjectFile>(Obj) || isa<COFFImportFile>(Obj))
     return false;
+  if (XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj))
+    return XCOFFObj->is64Bit();
+
   if (isa<WasmObjectFile>(Obj))
     return false;
   if (TapiFile *Tapi = dyn_cast<TapiFile>(&Obj))
     return Tapi->is64Bit();
   if (MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj))
     return MachO->is64Bit();
   return cast<ELFObjectFileBase>(Obj).getBytesInAddress() == 8;
 }
default:
     // Check for section symbol.
     if (Symb.isSectionDefinition())
       return 's';
   }
 
   return '?';
 }
 
+static char getSymbolNMTypeChar(XCOFFObjectFile &Obj, symbol_iterator I) {
+  Expected<uint32_t> TypeOrErr = I->getType();
+  if (!TypeOrErr) {
+    warning(TypeOrErr.takeError(), Obj.getFileName());
+    return '?';
+  }
+
+  uint32_t SymType = *TypeOrErr;
+
+  if (SymType == SymbolRef::ST_Debug)
+    return 'N';
+
+  if (SymType == SymbolRef::ST_File)
+    return 'f';
+
+  Expected<section_iterator> SymSecOrErr = I->getSection();
+  if (!SymSecOrErr) {
+    warning(SymSecOrErr.takeError(), Obj.getFileName());
+    return '?';
+  }
+
+  section_iterator SecIter = *SymSecOrErr;
+
+  if (SecIter == Obj.section_end())
+    return '?';
+
+  if (SecIter->isText())
+    return 't';
+
+  if (SecIter->isData())
+    return 'd';
+
+  if (SecIter->isBSS())
+    return 'b';
+
+  return '?';
+}
+
 static char getSymbolNMTypeChar(COFFImportFile &Obj) {
   switch (Obj.getCOFFImportHeader()->getType()) {
   case COFF::IMPORT_CODE:
     return 't';
   case COFF::IMPORT_DATA:
     return 'd';
   case COFF::IMPORT_CONST:
     return 'r';
static char getNMSectionTagAndName(SymbolicFile &Obj, basic_symbol_iterator I,
 
   char Ret = '?';
   if (Symflags & object::SymbolRef::SF_Absolute)
     Ret = 'a';
   else if (IRObjectFile *IR = dyn_cast<IRObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*IR, I);
   else if (COFFObjectFile *COFF = dyn_cast<COFFObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*COFF, I);
+  else if (XCOFFObjectFile *XCOFF = dyn_cast<XCOFFObjectFile>(&Obj))
+    Ret = getSymbolNMTypeChar(*XCOFF, I);
   else if (COFFImportFile *COFFImport = dyn_cast<COFFImportFile>(&Obj))
     Ret = getSymbolNMTypeChar(*COFFImport);
   else if (MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*MachO, I);
   else if (WasmObjectFile *Wasm = dyn_cast<WasmObjectFile>(&Obj))
     Ret = getSymbolNMTypeChar(*Wasm, I);
   else if (TapiFile *Tapi = dyn_cast<TapiFile>(&Obj))
     Ret = getSymbolNMTypeChar(*Tapi, I);
for (BasicSymbolRef Sym : Symbols) {
       // see if this symbol is a symbol from that section and if not skip it.
       if (Nsect && Nsect != getNsectInMachO(*MachO, Sym))
         continue;
       NMSymbol S = {};
       S.Size = 0;
       S.Address = 0;
       if (isa<ELFObjectFileBase>(&Obj))
         S.Size = ELFSymbolRef(Sym).getSize();
       if (PrintAddress && isa<ObjectFile>(Obj)) {
         SymbolRef SymRef(Sym);
         Expected<uint64_t> AddressOrErr = SymRef.getAddress();
         if (!AddressOrErr) {
           consumeError(AddressOrErr.takeError());
           break;
         }
         S.Address = *AddressOrErr;
       }