gopalshankar / address-sanitizer

Automatically exported from code.google.com/p/address-sanitizer
0 stars 0 forks source link

getShadowMapping is hard to extend for new platforms #267

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
The calculation of Mapping.Offset is fairly complicated. First there are 
several nested ternary operators:

  Mapping.Offset = IsAndroid ? 0 :
      (LongSize == 32 ?
        (IsMIPS32 ? kMIPS32_ShadowOffset32 :
          (IsFreeBSD ? kFreeBSD_ShadowOffset32 : kDefaultShadowOffset32)) :
       IsPPC64 ? kPPC64_ShadowOffset64 : kDefaultShadowOffset64);

Then there is this conditional which overrides the above calculation for some 
platforms, but there is no comment describing why:

  if (!IsAndroid && ClShort64BitOffset && IsX86_64 && !IsMacOSX) {
    assert(LongSize == 64);
    Mapping.Offset = (IsFreeBSD ?
                      kFreeBSD_ShadowOffset64 : kDefaultShort64bitShadowOffset);
  }

And then another which is also not entirely explained:

  if (!IsAndroid && ClMappingOffsetLog >= 0) {
    // Zero offset log is the special case.
    Mapping.Offset = (ClMappingOffsetLog == 0) ? 0 : 1ULL << ClMappingOffsetLog;
  }

It's unclear to me the best way to extend this for new targets.

Original issue reported on code.google.com by rgovostes on 22 Feb 2014 at 3:42

GoogleCodeExporter commented 9 years ago
The code is hairy indeed, but it is correct. 
If you want to refactor it and, add comments, and/or add a new platform
a patch is most welcome. 
https://code.google.com/p/address-sanitizer/wiki/HowToContribute

Original comment by konstant...@gmail.com on 22 Feb 2014 at 4:31

GoogleCodeExporter commented 9 years ago
Without comments on what the intended behavior is (other than, perhaps, commit 
log messages), it would be very difficult for a contributor to refactor this 
correctly.

Original comment by rgovostes on 22 Feb 2014 at 4:34

GoogleCodeExporter commented 9 years ago
Ok, fair enough. Let me look into simplifying the code next week. 
What platform are you interested in (just curious). 

Original comment by konstant...@gmail.com on 22 Feb 2014 at 4:54

GoogleCodeExporter commented 9 years ago
I have it partially working on ARM64/iOS but before committing the code there 
are (many) hacks that I introduced, such as one in this function, that need to 
be cleaned up.

Original comment by rgovostes on 22 Feb 2014 at 4:58

GoogleCodeExporter commented 9 years ago
http://llvm.org/viewvc/llvm-project?rev=202033&view=rev should improve the 
readability. Please send your patch for review when you are ready. 

Original comment by konstant...@gmail.com on 24 Feb 2014 at 1:48

GoogleCodeExporter commented 9 years ago
Looks good, thanks.

Original comment by rgovostes on 24 Feb 2014 at 8:51